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

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;

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