Re: [Libguestfs] [p2v PATCH 02/10] remove "--nbd=nbdkit-no-sa" option parsing

On Thu, Mar 31, 2022 at 09:22:03AM +0200, Laszlo Ersek wrote: > According to the first commit message in this series, remove support for > parsing "--nbd=nbdkit-no-sa". > > Note that there is a non-intuitive change in this patch: in the > test_nbd_servers() function, we remove the grepping for the "LISTEN_PID" > string in the "nbdkit" binary. In other words, we stop verifying whether > "nbdkit" indeed supports socket activation, we just assume it. > > The primary reason for this is that this check never actually worked when > the nbdkit binary was exposed from the root directory of an nbdkit build > tree! That binary is actually built from "wrapper.c", and it's only the > "server/nbdkit" binary that matches "LISTEN_PID". However, as "wrapper.c" > explains, "server/nbdkit" would use the system-wide nbdkit plugins, not > the just-built ones, therefore only the wrapper "nbdkit" executable can be > used -- and so the "LISTEN_PID" grep command has always been unable to > deal with that situation. Unless we remove that check too, with the rest > of this patch, virt-p2v refuses to start up -- as no usable nbd server is > found. > > This patch permits further simplification, which will be done subsequently > in this series. > > Ref: https://listman.redhat.com/archives/libguestfs/2022-March/028475.html > Suggested-by: Richard W.M. Jones <rjones(a)redhat.com&gt; > Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; You might want to add some information about when systemd socket activation was added to nbdkit: commit 7ff39d028c6359f5c0925ed2cf4a2c4c751af2e4 Author: Richard W.M. Jones <rjones(a)redhat.com&gt; Date: Tue Jan 31 16:00:05 2017 +0000 Add support for socket activation. First available in a release in nbdkit 1.1.13 (Aug 2017).

Rest of the patch is fine, so: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt;

Rich. > nbd.c | 42 +------------------- > test-virt-p2v-nbdkit.sh | 2 +- > virt-p2v.pod | 18 +++------ > 3 files changed, 8 insertions(+), 54 deletions(-) > > diff --git a/nbd.c b/nbd.c > index 46f6304d2c51..edc3ab51d889 100644 > --- a/nbd.c > +++ b/nbd.c > @@ -50,8 +50,7 @@ static int nbd_local_port; > /* List of servers specified by the --nbd option. */ > enum nbd_server { > /* 0 is reserved for "end of list" */ > NBDKIT = 1, > - NBDKIT_NO_SA = 2, > }; > static enum nbd_server *cmdline_servers = NULL; > > @@ -59,31 +58,29 @@ static const char * > nbd_server_string (enum nbd_server s) > { > const char *ret = NULL; > > switch (s) { > case NBDKIT: ret = "nbdkit"; break; > - case NBDKIT_NO_SA: ret = "nbdkit-no-sa"; break; > } > > if (ret == NULL) > abort (); > > return ret; > } > > /* If no --nbd option is passed, we use this standard list instead. > * Must match the documentation in virt-p2v(1). > */ > static const enum nbd_server standard_servers[] = > - { NBDKIT, NBDKIT_NO_SA, 0 }; > + { NBDKIT, 0 }; > > /* After testing the list of servers passed by the user, this is > * server we decide to use. > */ > static enum nbd_server use_server; > > static pid_t start_nbdkit (const char *device, const char *ipaddr, int port, int *fds, size_t nr_fds); > -static int get_local_port (void); > static int open_listening_socket (const char *ipaddr, int **fds, size_t *nr_fds); > static int bind_tcpip_socket (const char *ipaddr, const char *port, int **fds, size_t *nr_fds); > static int connect_with_source_port (const char *hostname, int dest_port, int source_port); > @@ -126,40 +123,38 @@ void > set_nbd_option (const char *opt) > { > size_t i, len; > CLEANUP_FREE_STRING_LIST char **strs = NULL; > > if (cmdline_servers != NULL) > error (EXIT_FAILURE, 0, _("--nbd option appears multiple times")); > > strs = guestfs_int_split_string (',', opt); > > if (strs == NULL) > error (EXIT_FAILURE, errno, _("malloc")); > > len = guestfs_int_count_strings (strs); > if (len == 0) > error (EXIT_FAILURE, 0, _("--nbd option cannot be empty")); > > cmdline_servers = malloc (sizeof (enum nbd_server) * (len + 1)); > if (cmdline_servers == NULL) > error (EXIT_FAILURE, errno, _("malloc")); > > for (i = 0; strs[i] != NULL; ++i) { > if (STREQ (strs[i], "nbdkit")) > cmdline_servers[i] = NBDKIT; > - else if (STREQ (strs[i], "nbdkit-no-sa")) > - cmdline_servers[i] = NBDKIT_NO_SA; > else > error (EXIT_FAILURE, 0, _("--nbd: unknown server: %s"), strs[i]); > } > > assert (i == len); > cmdline_servers[i] = 0; /* marks the end of the list */ > } > > /** > * Test the I<--nbd> option (or built-in default list) to see which > * servers are actually installed and appear to be working. > * > * Set the C<use_server> global accordingly. > */ > @@ -167,88 +162,75 @@ void > test_nbd_servers (void) > { > size_t i; > int r; > const enum nbd_server *servers; > > /* Initialize nbd_local_port. */ > if (is_iso_environment) > /* The p2v ISO should allow us to open up just about any port, so > * we can fix a port number in that case. Using a predictable > * port number in this case should avoid rare errors if the port > * colides with another (ie. it'll either always fail or never > * fail). > */ > nbd_local_port = 50123; > else > /* When testing on the local machine, choose a random port. */ > nbd_local_port = 50000 + (random () % 10000); > > if (cmdline_servers != NULL) > servers = cmdline_servers; > else > servers = standard_servers; > > use_server = 0; > > for (i = 0; servers[i] != 0; ++i) { > #if DEBUG_STDERR > fprintf (stderr, "checking for %s ...\n", nbd_server_string (servers[i])); > #endif > > switch (servers[i]) { > case NBDKIT: /* with socket activation */ > r = system ("nbdkit file --version" > #ifndef DEBUG_STDERR > " >/dev/null 2>&1" > -#endif > - " && grep -sq LISTEN_PID `which nbdkit`" > - ); > - if (r == 0) { > - use_server = servers[i]; > - goto finish; > - } > - break; > - > - case NBDKIT_NO_SA: > - r = system ("nbdkit file --version" > -#ifndef DEBUG_STDERR > - " >/dev/null 2>&1" > #endif > ); > if (r == 0) { > use_server = servers[i]; > goto finish; > } > break; > > default: > abort (); > } > } > > finish: > if (use_server == 0) { > fprintf (stderr, > _("%s: no working NBD server was found, cannot continue.\n" > "Please check the --nbd option in the virt-p2v(1) man page.\n"), > g_get_prgname ()); > exit (EXIT_FAILURE); > } > > /* Release memory used by the --nbd option. */ > free (cmdline_servers); > cmdline_servers = NULL; > > #if DEBUG_STDERR > fprintf (stderr, "picked %s\n", nbd_server_string (use_server)); > #endif > } > > /** > * Start the NBD server. > * > * We previously tested all NBD servers (see C<test_nbd_servers>) and > * hopefully found one which will work. > * > * Returns the process ID (E<gt> 0) or C<0> if there is an error. > */ > @@ -256,35 +238,29 @@ pid_t > start_nbd_server (const char **ipaddr, int *port, const char *device) > { > int *fds = NULL; > size_t i, nr_fds; > pid_t pid; > > switch (use_server) { > case NBDKIT: /* nbdkit with socket activation */ > *ipaddr = "localhost"; > *port = open_listening_socket (*ipaddr, &fds, &nr_fds); > if (*port == -1) return -1; > pid = start_nbdkit (device, *ipaddr, *port, fds, nr_fds); > for (i = 0; i < nr_fds; ++i) > close (fds[i]); > free (fds); > return pid; > - > - case NBDKIT_NO_SA: /* nbdkit without socket activation */ > - *ipaddr = "localhost"; > - *port = get_local_port (); > - if (*port == -1) return -1; > - return start_nbdkit (device, *ipaddr, *port, NULL, 0); > } > > abort (); > } > > #define FIRST_SOCKET_ACTIVATION_FD 3 > > /** > * Set up file descriptors and environment variables for > * socket activation. > * > * Note this function runs in the child between fork and exec. > */ > @@ -325,89 +301,73 @@ static pid_t > start_nbdkit (const char *device, > const char *ipaddr, int port, int *fds, size_t nr_fds) > { > pid_t pid; > char port_str[64]; > CLEANUP_FREE char *file_str = NULL; > > #if DEBUG_STDERR > fprintf (stderr, "starting nbdkit for %s on %s:%d%s\n", > device, ipaddr, port, > fds == NULL ? "" : " using socket activation"); > #endif > > snprintf (port_str, sizeof port_str, "%d", port); > > if (asprintf (&file_str, "file=%s", device) == -1) > error (EXIT_FAILURE, errno, "asprintf"); > > pid = fork (); > if (pid == -1) { > set_nbd_error ("fork: %m"); > return 0; > } > > if (pid == 0) { /* Child. */ > close (0); > if (open ("/dev/null", O_RDONLY) == -1) { > perror ("open: /dev/null"); > _exit (EXIT_FAILURE); > } > > if (fds == NULL) { /* without socket activation */ > execlp ("nbdkit", > "nbdkit", > "-r", /* readonly (vital!) */ > "-p", port_str, /* listening port */ > "-i", ipaddr, /* listen only on loopback interface */ > "-f", /* don't fork */ > "file", /* file plugin */ > file_str, /* a device like file=/dev/sda */ > NULL); > perror ("nbdkit"); > _exit (EXIT_FAILURE); > } > else { /* socket activation */ > socket_activation (fds, nr_fds); > > execlp ("nbdkit", > "nbdkit", > "-r", /* readonly (vital!) */ > "-f", /* don't fork */ > "file", /* file plugin */ > file_str, /* a device like file=/dev/sda */ > NULL); > perror ("nbdkit"); > _exit (EXIT_FAILURE); > } > } > > /* Parent. */ > return pid; > } > > -/** > - * This is used when we are starting an NBD server that does not > - * support socket activation. We have to pass the '-p' option to > - * the NBD server, but there's no good way to choose a free port, > - * so we have to just guess. > - * > - * Returns the port number on success or C<-1> on error. > - */ > -static int > -get_local_port (void) > -{ > - int port = nbd_local_port; > - nbd_local_port++; > - return port; > -} > - > /** > * This is used when we are starting an NBD server which supports > * socket activation. We can open a listening socket on an unused > * local port and return it. > * > * Returns the port number on success or C<-1> on error. > * > * The file descriptor(s) bound are returned in the array *fds, *nr_fds. > * The caller must free the array. > */ > diff --git a/test-virt-p2v-nbdkit.sh b/test-virt-p2v-nbdkit.sh > index 8e91d45c4014..99bb8d9fe293 100755 > --- a/test-virt-p2v-nbdkit.sh > +++ b/test-virt-p2v-nbdkit.sh > @@ -47,7 +47,7 @@ export PATH=$d:$PATH > cmdline="p2v.server=localhost p2v.name=fedora p2v.disks=$f1,$f2 p2v.o=local p2v.os=$(pwd)/$d p2v.network=em1:wired,other p2v.post=" > > # Only use nbdkit. > -$VG virt-p2v --cmdline="$cmdline" --nbd=nbdkit,nbdkit-no-sa > +$VG virt-p2v --cmdline="$cmdline" --nbd=nbdkit > > # Test the libvirt XML metadata and a disk was created. > test -f $d/fedora.xml > diff --git a/virt-p2v.pod b/virt-p2v.pod > index eb220cbf1cee..e6cbb1514edc 100644 > --- a/virt-p2v.pod > +++ b/virt-p2v.pod > @@ -509,24 +509,17 @@ features such as the Shutdown popup button. > =item B<--nbd=server[,server...]> > > Select which NBD server is used. By default the following servers are > -checked and the first one found is used: I<--nbd=nbdkit,nbdkit-no-sa> > +checked and the first one found is used: I<--nbd=nbdkit>. > > =over 4 > > =item B<nbdkit> > > -Use nbdkit with the file plugin (see: L<nbdkit-file-plugin(1)>). > - > -=item B<nbdkit-no-sa> > - > -Use nbdkit, but disable socket activation > +Use nbdkit with the file plugin (see: L<nbdkit-file-plugin(1)>) and > +socket activation. > > =back > > -The C<nbdkit-no-sa> variant allows virt-p2v to fall back to older > -versions of nbdkit which did not support L<socket > -activation|http://0pointer.de/blog/projects/socket-activation.html>. > - > =item B<--test-disk=/PATH/TO/DISK.IMG> > > For testing or debugging purposes, replace F</dev/sda> with a local > @@ -670,8 +663,9 @@ Before conversion actually begins, virt-p2v then makes one or more > further ssh connections to the server for data transfer. > > The transfer protocol used currently is NBD (Network Block Device), > -which is proxied over ssh. The NBD server is L<nbdkit(1)>; socket > -activation can be controlled using the I<--nbd> command line option. > +which is proxied over ssh. The NBD server is L<nbdkit(1)>, with > +L<nbdkit-file-plugin(1)> and L<socket > +activation|http://0pointer.de/blog/projects/socket-activation.html>. > > There is one ssh connection per physical hard disk on the source > machine (the common case — a single hard disk — is shown below): > -- > 2.19.1.3.g30247aa5d201 > > > _______________________________________________ > Libguestfs mailing list > Libguestfs(a)redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs