[Libguestfs] [p2v PATCH 02/10] remove "--nbd=nbdkit-no-sa" option parsing
Laszlo Ersek
lersek at redhat.com
Thu Mar 31 13:25:30 UTC 2022
On 03/31/22 14:52, Richard W.M. Jones wrote:
> 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 at redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>
> You might want to add some information about when systemd socket
> activation was added to nbdkit:
>
> commit 7ff39d028c6359f5c0925ed2cf4a2c4c751af2e4
> Author: Richard W.M. Jones <rjones at redhat.com>
> 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).
Will do.
>
> Rest of the patch is fine, so:
>
> Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
Thanks!
Laszlo
>
> 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 at redhat.com
>> https://listman.redhat.com/mailman/listinfo/libguestfs
>
More information about the Libguestfs
mailing list