Laszlo Ersek
2022-Mar-31 07:22 UTC
[Libguestfs] [p2v PATCH 00/10] Simplify NBD server management
Repo: https://github.com/lersek/virt-p2v.git Branch: nbd-servers-simplification Rich proposed a list of tasks at <https://listman.redhat.com/archives/libguestfs/2022-March/028475.html>, for simplifying NBD server management. This series implements them. Earlier I said I'd post this as an RFC series, but after reviewing and testing the set myself, I decided to post it as a PATCH series. The set builds at every stage. I verified the documentation ("docs/p2v-building.1" and "virt-p2v.1") at every stage that modifies the sources of those files, too. The set passes make check I also tested it with make run-virt-p2v-in-an-nvme-vm i.e. an actual conversion, running against a RHEL9 conversion server (virt-v2v 1.45.99rhel=9,release=1.el9; nbdkit 1.30.1-1.el9.x86_64). The converted guest boots fine. The conversion log is available if needed. On the p2v (virtual) machine, I saved the interesting messages from the serial console (they are interesting because they relate to code modified by this series):> [ 232.205162] launch-virt-p2v[669]: checking for nbdkit ... > [ 233.209388] launch-virt-p2v[675]: nbdkit 1.28.6 (nbdkit-1.28.6-1.fc35) > [ 233.213329] launch-virt-p2v[675]: file 1.28.6 > [ 233.216906] launch-virt-p2v[669]: found nbdkit > > [ 301.197612] launch-virt-p2v[669]: virt-p2v: bound to localhost:50123 (2 socket(s)) > [ 301.198566] launch-virt-p2v[669]: starting nbdkit for /dev/nvme0n1 using socket activation > [ 301.220919] launch-virt-p2v[669]: ssh command: ssh -p 22 -l root -o StrictHostKeyChecking=no -o ConnectTimeout=60 -o ServerAliveInterval=300 -o ServerAliveCountMax=6 -o PreferredAuthentications=keyboard-interactive,password -R 0:localhost:50123 -N 192.168.122.184 > [ 301.225792] launch-virt-p2v[748]: nbdkit: file[1]: error: reading initial client flags: conn->recv: Connection reset by peer > > [ 301.583073] launch-virt-p2v[669]: virt-p2v: data connection for /dev/nvme0n1: SSH remote port 44479, local port 50123 > > [ 301.588509] launch-virt-p2v[669]: virt-p2v: bound to localhost:50124 (2 socket(s)) > [ 301.592034] launch-virt-p2v[669]: starting nbdkit for /dev/nvme1n1 using socket activation > [ 301.605749] launch-virt-p2v[669]: ssh command: ssh -p 22 -l root -o StrictHostKeyChecking=no -o ConnectTimeout=60 -o ServerAliveInterval=300 -o ServerAliveCountMax=6 -o PreferredAuthentications=keyboard-interactive,password -R 0:localhost:50124 -N 192.168.122.184 > [ 301.612459] launch-virt-p2v[751]: nbdkit: file[1]: error: reading initial client flags: conn->recv: Connection reset by peer > > [ 302.002998] launch-virt-p2v[669]: virt-p2v: data connection for /dev/nvme1n1: SSH remote port 35789, local port 50124 > > [ 302.007171] launch-virt-p2v[669]: virt-p2v: bound to localhost:50125 (2 socket(s)) > [ 302.009976] launch-virt-p2v[669]: starting nbdkit for /dev/nvme1n2 using socket activation > [ 302.020637] launch-virt-p2v[669]: ssh command: ssh -p 22 -l root -o StrictHostKeyChecking=no -o ConnectTimeout=60 -o ServerAliveInterval=300 -o ServerAliveCountMax=6 -o PreferredAuthentications=keyboard-interactive,password -R 0:localhost:50125 -N 192.168.122.184 > [ 302.026882] launch-virt-p2v[754]: nbdkit: file[1]: error: reading initial client flags: conn->recv: Connection reset by peer > > [ 302.410966] launch-virt-p2v[669]: virt-p2v: data connection for /dev/nvme1n2: SSH remote port 38663, local port 50125I formatted the series with "-W" (aka "--function-context"), for easier review. Some patches are best viewed with "-b" (aka "--ignore-space-change"), for masking un-indentation; I did not use that option for posting this series, as it could interfere with applying the patches from the list. For looking at some of the patches like that, fetch the series from the URL at the top (or apply it from the list), and run "git show -b" locally. Thanks, Laszlo Laszlo Ersek (10): remove qemu-nbd support remove "--nbd=nbdkit-no-sa" option parsing nbd.c: simplify start_nbdkit() open-code "localhost" as the listen hostname for nbdkit nbd.c: remove bind_source_port() remove "--nbd" option parsing nbd.c: remove nbd_server_string() nbd.c: remove "enum nbd_server" rely on Linux for killing nbdkit, when the parent thread exits nbd.c: bind listening socket without AI_ADDRCONFIG Makefile.am | 2 - contrib/aux-scripts/do-build.sh | 7 +- contrib/patches/0002-RHEL-5-ONLY-QEMU-NBD-1.4-HAS-NO-f-OPTION.patch | 34 -- conversion.c | 23 +- dependencies.m4 | 6 +- docs/p2v-building.pod | 9 +- main.c | 7 +- nbd.c | 514 +++----------------- p2v.h | 10 +- ssh.c | 6 +- test-virt-p2v-nbdkit.sh | 6 +- test-virt-p2v-scp.sh | 2 +- test-virt-p2v-ssh.sh | 4 +- test-virt-p2v.sh | 56 --- virt-p2v.pod | 49 +- 15 files changed, 109 insertions(+), 626 deletions(-) delete mode 100644 contrib/patches/0002-RHEL-5-ONLY-QEMU-NBD-1.4-HAS-NO-f-OPTION.patch delete mode 100755 test-virt-p2v.sh base-commit: c1a4bd83a8bba5959470ef1f6deb06dc61587112 -- 2.19.1.3.g30247aa5d201
On 03/24/22 10:13, Richard W.M. Jones wrote:> virt-p2v needs an NBD server running on the p2v machine to export the > disks. Originally we used qemu-nbd since that was the only choice. > In 2017, I added support for nbdkit as an alternative to qemu-nbd. > > So now we're in the situation where either server can be used (see > virt-p2v.git/nbd.c). For added complexity we also support both > servers in either systemd socket activation (SA) mode or "no-SA" mode, > so that's 4 combinations. > > This is silly, we should support only one NBD server, and since > systemd socket activation is well-supported and more flexible, we > should just use it. > > So the question is *which* NBD server to support. That's not so much > a technical matter since both servers can easily serve a local block > device (always raw format). However I do think that nbdkit might > genuinely be the better choice here: > > - qemu-nbd links to the whole qemu block layer, nbdkit can be shipped > with just the plugin we need, so it should be smaller with less > code surface > > - nbdkit-file-plugin has a better method of not trashing the host > page cache > > - could use nbdkit --exit-with-parent feature (which we don't at the > moment) > > - nbdkit is widely available in distros these days > > Also that file uses AI_ADDRCONFIG so I guess it has problems with IPv6.This patch removes qemu-nbd support, as the first step. "test-virt-p2v.sh" is simply removed, as the remaining "test-virt-p2v-nbdkit.sh" is mostly identical. 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> --- Makefile.am | 2 - contrib/aux-scripts/do-build.sh | 7 +- contrib/patches/0002-RHEL-5-ONLY-QEMU-NBD-1.4-HAS-NO-f-OPTION.patch | 34 ----- dependencies.m4 | 6 +- docs/p2v-building.pod | 9 +- main.c | 2 +- nbd.c | 136 +------------------- test-virt-p2v-nbdkit.sh | 5 +- test-virt-p2v-scp.sh | 2 +- test-virt-p2v-ssh.sh | 4 +- test-virt-p2v.sh | 56 -------- virt-p2v.pod | 29 ++--- 12 files changed, 31 insertions(+), 261 deletions(-) diff --git a/Makefile.am b/Makefile.am index 2080890b14e3..e98129e99028 100644 --- a/Makefile.am +++ b/Makefile.am @@ -27,36 +27,35 @@ SUBDIRS += bash EXTRA_DIST = \ $(BUILT_SOURCES) \ $(TESTS) $(LIBGUESTFS_TESTS) $(SLOW_TESTS) \ AUTHORS \ contrib/aux-scripts/do-build.sh \ contrib/build-p2v-iso.sh \ contrib/patches/0001-RHEL-5-ONLY-DISABLE-AUTOMATIC-REMOTE-PORT-ALLOCATION.patch \ - contrib/patches/0002-RHEL-5-ONLY-QEMU-NBD-1.4-HAS-NO-f-OPTION.patch \ contrib/test-p2v-iso.sh \ dependencies.m4 \ generate-p2v-authors.pl \ generate-p2v-config.pl \ issue \ kiwi-config.sh \ kiwi-config.xml.in \ launch-virt-p2v \ libguestfs/README \ miniexpect/README \ p2v.ks.in \ p2v.service \ podcheck.pl \ test-functions.sh \ test-virt-p2v-docs.sh \ test-virt-p2v-pxe.sshd_config.in \ test-virt-p2v-scp.sh \ test-virt-p2v-ssh.sh \ virt-p2v.pod \ virt-p2v-make-disk.in \ virt-p2v-make-disk.pod \ virt-p2v-make-kickstart.in \ virt-p2v-make-kickstart.pod \ virt-p2v-make-kiwi.in \ virt-p2v-make-kiwi.pod # Don't clean ssh_host_rsa_key{,.pub} or id_rsa{,.pub} since those # consume system entropy to regenerate. @@ -339,7 +338,6 @@ TESTS = \ test-virt-p2v-docs.sh LIBGUESTFS_TESTS = \ - test-virt-p2v.sh \ test-virt-p2v-nbdkit.sh if HAVE_LIBGUESTFS diff --git a/contrib/aux-scripts/do-build.sh b/contrib/aux-scripts/do-build.sh index 53a844d9c99d..0ef2ab185beb 100644 --- a/contrib/aux-scripts/do-build.sh +++ b/contrib/aux-scripts/do-build.sh @@ -49,22 +49,21 @@ pushd libguestfs-* # Various hacks for different versions of RHEL. case $osversion in rhel-5.*|centos-5.*) # This just forces configure to ignore these missing dependencies. export LIBTINFO_CFLAGS=-D_GNU_SOURCE export LIBTINFO_LIBS=-lncurses export JANSSON_CFLAGS=-D_GNU_SOURCE export JANSSON_LIBS=-ljansson # Remove some unsupported flags that the configure script hard codes. sed -i -e 's/-fno-strict-overflow//' configure sed -i -e 's/-Wno-strict-overflow//' configure # Apply some RHEL 5 only patches. patch -p1 < ../patches/0001-RHEL-5-ONLY-DISABLE-AUTOMATIC-REMOTE-PORT-ALLOCATION.patch - patch -p1 < ../patches/0002-RHEL-5-ONLY-QEMU-NBD-1.4-HAS-NO-f-OPTION.patch ;; rhel-6.*|centos-6.*) # This just forces configure to ignore these missing dependencies. export LIBTINFO_CFLAGS=-D_GNU_SOURCE export LIBTINFO_LIBS=-lncurses export JANSSON_CFLAGS=-D_GNU_SOURCE export JANSSON_LIBS=-ljansson ;; @@ -106,41 +105,43 @@ popd # More hacks for different versions of RHEL. case $osversion in rhel-5.*|centos-5.*) # RHEL 5 livecd-tools is broken with syslinux, this fixes it: sed -i -e 's,/usr/lib/syslinux/,/usr/share/syslinux/,g'\ /usr/lib/python2.4/site-packages/imgcreate/live.py # livecd-tools cannot parse certain aspects of the kickstart: sed -i \ -e 's/--plaintext//g' \ -e 's/^firewall.*//g' \ -e 's/^%end.*//g' \ p2v.ks # Remove some packages which don't exist on RHEL 5: sed -i \ -e 's,^dracut-live.*,,g' \ -e 's,^dejavu-.*,,g' \ -e 's,^mesa-dri-drivers.*,,g' \ -e 's,^network-manager-applet.*,,g' \ -e 's,^nm-connection-editor.*,,g' \ - -e 's,^/usr/bin/qemu-nbd.*,,g' \ + -e 's,^/usr/sbin/nbdkit.*,,g' \ + -e 's,^/usr/lib64/nbdkit/plugins/nbdkit-file-plugin.so.*,,g' \ -e '/^net-tools/a syslinux' \ p2v.ks # Remove systemctl lines, doesn't exist on RHEL 5. sed -i \ -e 's/^\(systemctl.*\)/#\1/g' \ p2v.ks ;; rhel-6.*|centos-6.*) # Remove some packages which don't exist on RHEL 6: sed -i \ -e 's,^dracut-live.*,,g' \ -e 's,^firewalld.*,,g' \ -e 's,^network-manager-applet.*,,g' \ -e 's,^nm-connection-editor.*,,g' \ - -e 's,^/usr/bin/qemu-nbd.*,,g' \ + -e 's,^/usr/sbin/nbdkit.*,,g' \ + -e 's,^/usr/lib64/nbdkit/plugins/nbdkit-file-plugin.so.*,,g' \ p2v.ks # Remove systemctl lines, doesn't exist on RHEL 5. sed -i \ -e 's/^\(systemctl.*\)/#\1/g' \ p2v.ks ;; diff --git a/contrib/patches/0002-RHEL-5-ONLY-QEMU-NBD-1.4-HAS-NO-f-OPTION.patch b/contrib/patches/0002-RHEL-5-ONLY-QEMU-NBD-1.4-HAS-NO-f-OPTION.patch deleted file mode 100644 index d0bc2cfc9ec2..000000000000 --- a/contrib/patches/0002-RHEL-5-ONLY-QEMU-NBD-1.4-HAS-NO-f-OPTION.patch +++ /dev/null @@ -1,34 +0,0 @@ -From 3ccd25c981431426038d7952f5b0b86118d92c23 Mon Sep 17 00:00:00 2001 -From: "Richard W.M. Jones" <rjones at redhat.com> -Date: Sat, 21 Jan 2017 05:57:17 -0500 -Subject: [PATCH 2/2] RHEL 5 ONLY QEMU-NBD 1.4 HAS NO -f OPTION - ---- - p2v/nbd.c | 4 ++-- - 1 file changed, 2 insertions(+), 2 deletions(-) - -diff --git a/p2v/nbd.c b/p2v/nbd.c -index b1caf2f..2232b08 100644 ---- a/p2v/nbd.c -+++ b/p2v/nbd.c -@@ -409,7 +409,7 @@ start_qemu_nbd (const char *device, - "-r", /* readonly (vital!) */ - "-p", port_str, /* listening port */ - "-t", /* persistent */ -- "-f", "raw", /* force raw format */ -+ //"-f", "raw", /* force raw format */ - "-b", ipaddr, /* listen only on loopback interface */ - "--cache=unsafe", /* use unsafe caching for speed */ - device, /* a device like /dev/sda */ -@@ -424,7 +424,7 @@ start_qemu_nbd (const char *device, - "qemu-nbd", - "-r", /* readonly (vital!) */ - "-t", /* persistent */ -- "-f", "raw", /* force raw format */ -+ //"-f", "raw", /* force raw format */ - "--cache=unsafe", /* use unsafe caching for speed */ - device, /* a device like /dev/sda */ - NULL); --- -1.8.2.3 - diff --git a/dependencies.m4 b/dependencies.m4 index 40b3786cb0b7..20d068028dbe 100644 --- a/dependencies.m4 +++ b/dependencies.m4 @@ -23,39 +23,40 @@ dnl only runs on the virt-v2v conversion server. ifelse(REDHAT,1, dnl Used by the virt-p2v binary. pcre libxml2 gtk`'GTK_VERSION dbus-libs dnl Run as external programs by the p2v binary. /usr/bin/ssh - /usr/bin/qemu-nbd + /usr/sbin/nbdkit + /usr/lib64/nbdkit/plugins/nbdkit-file-plugin.so which dnl Generally useful tools to use within xterm vim-minimal dnl Useful disk and diagnostic utilities. iscsi-initiator-utils dnl X11 environment /usr/bin/xinit /usr/bin/Xorg xorg-x11-drivers xorg-x11-fonts-Type1 dejavu-sans-fonts dejavu-sans-mono-fonts mesa-dri-drivers metacity NetworkManager nm-connection-editor network-manager-applet dnl dbus is required by nm-applet, but not a dependency in Fedora dbus-x11 dnl sysadmins prefer ifconfig net-tools dnl RHBZ#1157679 @hardware-support ) @@ -128,33 +129,34 @@ ifelse(SUSE,1, ifelse(OPENMANDRIVA,1, dnl Used by the virt-p2v binary. pcre libxml2 gtk`'GTK_VERSION dbus-libs dnl Run as external programs by the p2v binary. /usr/bin/ssh - /usr/bin/qemu-nbd + /usr/sbin/nbdkit + /usr/lib64/nbdkit/plugins/nbdkit-file-plugin.so which dnl Generally useful tools to use within xterm vim-enhanced dnl X11 environment /usr/bin/xinit /usr/bin/Xorg xorg-x11-drivers xorg-x11-fonts-Type1 dejavu-sans-fonts dejavu-sans-mono-fonts mesa-dri-drivers kwin_x11 NetworkManager nm-connection-editor network-manager-applet dnl dbus is required by nm-applet, but not a dependency in Fedora dbus-x11 dnl sysadmins prefer ifconfig net-tools ) diff --git a/docs/p2v-building.pod b/docs/p2v-building.pod index f249f742554a..21edc0255015 100644 --- a/docs/p2v-building.pod +++ b/docs/p2v-building.pod @@ -73,15 +73,12 @@ I<Required>. I<Required>. -=item qemu-nbd - =item nbdkit -Optional. qemu-nbd is used for testing. +Optional. -L<virt-p2v(1)> requires either qemu-nbd or nbdkit, but these only need -to be present on the virt-p2v ISO, they do not need to be installed at -compile time. +L<virt-p2v(1)> requires nbdkit, but it only needs to be present on the +virt-p2v ISO, it does not need to be installed at compile time. =item Gtk E<ge> 2.24, or 3 diff --git a/main.c b/main.c index a28a8b64ddaa..e4e9c1fe21c1 100644 --- a/main.c +++ b/main.c @@ -87,27 +87,27 @@ static void __attribute__((noreturn)) usage (int status) { if (status != EXIT_SUCCESS) fprintf (stderr, _("Try ?%s --help? for more information.\n"), g_get_prgname ()); else { printf (_("%s: Convert a physical machine to use KVM\n" "Copyright (C) 2009-2019 Red Hat Inc.\n" "Usage:\n" " %s [--options]\n" "Options:\n" " --help Display brief help\n" " --cmdline=CMDLINE Used to debug command line parsing\n" " --colors|--colours Use ANSI colour sequences even if not tty\n" " --iso Running in the ISO environment\n" - " --nbd=qemu-nbd,nbdkit Search order for NBD servers\n" + " --nbd=nbdkit Search order for NBD servers\n" " --test-disk=DISK.IMG For testing, use disk as /dev/sda\n" " -v|--verbose Verbose messages\n" " -V|--version Display version and exit\n" "For more information, see the manpage %s(1).\n"), g_get_prgname (), g_get_prgname (), g_get_prgname ()); } exit (status); } /* XXX Copied from fish/options.c. */ diff --git a/nbd.c b/nbd.c index db6228a8aba0..46f6304d2c51 100644 --- a/nbd.c +++ b/nbd.c @@ -1,47 +1,47 @@ /* virt-p2v * Copyright (C) 2009-2019 Red Hat Inc. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation; either version 2 of the License, or * (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program. If not, see <https://www.gnu.org/licenses/>. */ /** * This file handles the virt-p2v I<--nbd> command line option - * and running either L<qemu-nbd(8)> or L<nbdkit(1)>. + * and running L<nbdkit(1)>. */ #include <config.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <inttypes.h> #include <fcntl.h> #include <unistd.h> #include <time.h> #include <netdb.h> #include <errno.h> #include <error.h> #include <libintl.h> #include <sys/types.h> #include <sys/socket.h> #include <assert.h> #include "p2v.h" /* How long to wait for the NBD server to start (seconds). */ #define WAIT_NBD_TIMEOUT 10 /* The local port that the NBD server listens on (incremented for * each server which is started). */ @@ -50,10 +50,8 @@ static int nbd_local_port; /* List of servers specified by the --nbd option. */ enum nbd_server { /* 0 is reserved for "end of list" */ - QEMU_NBD = 1, - QEMU_NBD_NO_SA = 2, - NBDKIT = 3, - NBDKIT_NO_SA = 4, + NBDKIT = 1, + NBDKIT_NO_SA = 2, }; static enum nbd_server *cmdline_servers = NULL; @@ -61,32 +59,29 @@ static const char * nbd_server_string (enum nbd_server s) { const char *ret = NULL; switch (s) { - case QEMU_NBD: ret = "qemu-nbd"; break; - case QEMU_NBD_NO_SA: ret = "qemu-nbd-no-sa"; break; 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[] - { QEMU_NBD, QEMU_NBD_NO_SA, NBDKIT, NBDKIT_NO_SA, 0 }; + { NBDKIT, NBDKIT_NO_SA, 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_qemu_nbd (const char *device, const char *ipaddr, int port, int *fds, size_t nr_fds); 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); @@ -131,44 +126,40 @@ 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], "qemu-nbd") || STREQ (strs[i], "qemu")) - cmdline_servers[i] = QEMU_NBD; - else if (STREQ (strs[i], "qemu-nbd-no-sa") || STREQ (strs[i], "qemu-no-sa")) - cmdline_servers[i] = QEMU_NBD_NO_SA; - else if (STREQ (strs[i], "nbdkit")) + 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. */ @@ -176,113 +167,88 @@ 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 QEMU_NBD: /* with socket activation */ - r = system ("qemu-nbd --version" -#ifndef DEBUG_STDERR - " >/dev/null 2>&1" -#endif - " && grep -sq LISTEN_PID `which qemu-nbd`" - ); - if (r == 0) { - use_server = servers[i]; - goto finish; - } - break; - - case QEMU_NBD_NO_SA: - r = system ("qemu-nbd --version" -#ifndef DEBUG_STDERR - " >/dev/null 2>&1" -#endif - ); - if (r == 0) { - use_server = servers[i]; - goto finish; - } - break; - 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. */ @@ -290,56 +256,35 @@ 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 QEMU_NBD: /* qemu-nbd with socket activation */ - /* Ideally we would bind this socket to "localhost", but that - * requires two listening FDs, and qemu-nbd currently cannot - * support socket activation with two FDs. So we only bind to the - * IPv4 address. - */ - *ipaddr = "127.0.0.1"; - *port = open_listening_socket (*ipaddr, &fds, &nr_fds); - if (*port == -1) return -1; - pid = start_qemu_nbd (device, *ipaddr, *port, fds, nr_fds); - for (i = 0; i < nr_fds; ++i) - close (fds[i]); - free (fds); - return pid; - - case QEMU_NBD_NO_SA: /* qemu-nbd without socket activation */ - *ipaddr = "localhost"; - *port = get_local_port (); - if (*port == -1) return -1; - return start_qemu_nbd (device, *ipaddr, *port, NULL, 0); - 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. */ @@ -347,103 +292,32 @@ static inline void socket_activation (int *fds, size_t nr_fds) { size_t i; char nr_fds_str[16]; char pid_str[16]; if (fds == NULL) return; for (i = 0; i < nr_fds; ++i) { int fd = FIRST_SOCKET_ACTIVATION_FD + i; if (fds[i] != fd) { dup2 (fds[i], fd); close (fds[i]); } } snprintf (nr_fds_str, sizeof nr_fds_str, "%zu", nr_fds); setenv ("LISTEN_FDS", nr_fds_str, 1); snprintf (pid_str, sizeof pid_str, "%d", (int) getpid ()); setenv ("LISTEN_PID", pid_str, 1); } -/** - * Start a local L<qemu-nbd(1)> process. - * - * If we are using socket activation, C<fds> and C<nr_fds> will - * contain the locally pre-opened file descriptors for this. - * Otherwise if C<fds == NULL> we pass the port number. - * - * Returns the process ID (E<gt> 0) or C<0> if there is an error. - */ -static pid_t -start_qemu_nbd (const char *device, - const char *ipaddr, int port, int *fds, size_t nr_fds) -{ - pid_t pid; - char port_str[64]; - -#if DEBUG_STDERR - fprintf (stderr, "starting qemu-nbd for %s on %s:%d%s\n", - device, ipaddr, port, - fds == NULL ? "" : " using socket activation"); -#endif - - snprintf (port_str, sizeof port_str, "%d", port); - - 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 ("qemu-nbd", - "qemu-nbd", - "-r", /* readonly (vital!) */ - "-p", port_str, /* listening port */ - "-t", /* persistent */ - "-f", "raw", /* force raw format */ - "-b", ipaddr, /* listen only on loopback interface */ - "--cache=unsafe", /* use unsafe caching for speed */ - device, /* a device like /dev/sda */ - NULL); - perror ("qemu-nbd"); - _exit (EXIT_FAILURE); - } - else { /* socket activation */ - socket_activation (fds, nr_fds); - - execlp ("qemu-nbd", - "qemu-nbd", - "-r", /* readonly (vital!) */ - "-t", /* persistent */ - "-f", "raw", /* force raw format */ - "--cache=unsafe", /* use unsafe caching for speed */ - device, /* a device like /dev/sda */ - NULL); - perror ("qemu-nbd"); - _exit (EXIT_FAILURE); - } - } - - /* Parent. */ - return pid; -} - /** * Start a local L<nbdkit(1)> process using the * L<nbdkit-file-plugin(1)>. * * If we are using socket activation, C<fds> and C<nr_fds> will * contain the locally pre-opened file descriptors for this. * Otherwise if C<fds == NULL> we pass the port number. * * Returns the process ID (E<gt> 0) or C<0> if there is an error. */ diff --git a/test-virt-p2v-nbdkit.sh b/test-virt-p2v-nbdkit.sh index 9adb195f9249..8e91d45c4014 100755 --- a/test-virt-p2v-nbdkit.sh +++ b/test-virt-p2v-nbdkit.sh @@ -1,28 +1,27 @@ #!/bin/bash - # libguestfs virt-p2v test script # Copyright (C) 2014-2019 Red Hat Inc. # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by # the Free Software Foundation; either version 2 of the License, or # (at your option) any later version. # # This program is distributed in the hope that it will be useful, # but WITHOUT ANY WARRANTY; without even the implied warranty of # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU General Public License for more details. # # You should have received a copy of the GNU General Public License # along with this program. If not, see <https://www.gnu.org/licenses/>. -# Test virt-p2v in non-GUI mode using nbdkit instead of qemu-nbd. +# Test virt-p2v in non-GUI mode using nbdkit. set -e $TEST_FUNCTIONS skip_if_skipped skip_if_backend uml -skip_unless nbdkit file --version skip_unless test -f fedora.img skip_unless test -f blank-part.img @@ -47,7 +46,7 @@ export PATH=$d:$PATH # The Linux kernel command line. 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, disable qemu-nbd. +# Only use nbdkit. $VG virt-p2v --cmdline="$cmdline" --nbd=nbdkit,nbdkit-no-sa # Test the libvirt XML metadata and a disk was created. diff --git a/test-virt-p2v-scp.sh b/test-virt-p2v-scp.sh index ca851012fb85..65ed018cdfb8 100755 --- a/test-virt-p2v-scp.sh +++ b/test-virt-p2v-scp.sh @@ -1,20 +1,20 @@ #!/bin/bash - # Copyright (C) 2014-2019 Red Hat Inc. # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by # the Free Software Foundation; either version 2 of the License, or # (at your option) any later version. # # This program is distributed in the hope that it will be useful, # but WITHOUT ANY WARRANTY; without even the implied warranty of # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU General Public License for more details. # # You should have received a copy of the GNU General Public License # along with this program. If not, see <https://www.gnu.org/licenses/>. -# This is an scp substitute used by test-virt-p2v.sh. +# This is an scp substitute used by test-virt-p2v-nbdkit.sh. TEMP=`getopt \ -o 'o:P:' \ diff --git a/test-virt-p2v-ssh.sh b/test-virt-p2v-ssh.sh index 2442b7d3f2c6..8a14b71fbd4b 100755 --- a/test-virt-p2v-ssh.sh +++ b/test-virt-p2v-ssh.sh @@ -1,20 +1,20 @@ #!/bin/bash - # Copyright (C) 2014 Red Hat Inc. # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by # the Free Software Foundation; either version 2 of the License, or # (at your option) any later version. # # This program is distributed in the hope that it will be useful, # but WITHOUT ANY WARRANTY; without even the implied warranty of # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU General Public License for more details. # # You should have received a copy of the GNU General Public License # along with this program. If not, see <https://www.gnu.org/licenses/>. -# This is an ssh substitute used by test-virt-p2v.sh. +# This is an ssh substitute used by test-virt-p2v-nbdkit.sh. TEMP=`getopt \ -o 'l:No:p:R:' \ @@ -28,29 +28,29 @@ eval set -- "$TEMP" while true ; do case "$1" in # Regular arguments that we can just ignore. -N) shift ;; -l|-o|-p) shift 2 ;; # ssh -R 0:localhost:<port> (port forwarding). Don't actually # port forward, just return the original port number here so that - # the conversion process connects directly to qemu-nbd. + # the conversion process connects directly to nbdkit. -R) arg="$2" port="$(echo $arg | awk -F: '{print $3}')" echo "Allocated port" $port "for remote forward" shift 2 ;; --) shift break ;; *) echo "$0: internal error ($1)" exit 1 ;; esac diff --git a/test-virt-p2v.sh b/test-virt-p2v.sh deleted file mode 100755 index 9629e1b0235a..000000000000 --- a/test-virt-p2v.sh +++ /dev/null @@ -1,56 +0,0 @@ -#!/bin/bash - -# libguestfs virt-p2v test script -# Copyright (C) 2014 Red Hat Inc. -# -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 2 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program. If not, see <https://www.gnu.org/licenses/>. - -# Test virt-p2v in non-GUI mode. - -set -e - -$TEST_FUNCTIONS -skip_if_skipped -skip_if_backend uml -skip_unless test -f fedora.img -skip_unless test -f blank-part.img - -f1="$abs_builddir/fedora.img" -f2="$abs_builddir/blank-part.img" - -d=test-virt-p2v.d -rm -rf $d -mkdir $d - -# We don't want the program under test to run real 'ssh' or 'scp'. -# They won't work. Therefore create dummy 'ssh' and 'scp' binaries. -pushd $d -ln -sf "$abs_srcdir/test-virt-p2v-ssh.sh" ssh -ln -sf "$abs_srcdir/test-virt-p2v-scp.sh" scp -popd -export PATH=$d:$PATH - -# Note that the PATH already contains the local virt-p2v & virt-v2v -# binaries under test (because of the ./run script). - -# The Linux kernel command line. -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=" - -$VG virt-p2v --cmdline="$cmdline" - -# Test the libvirt XML metadata and a disk was created. -test -f $d/fedora.xml -test -f $d/fedora-sda -test -f $d/fedora-sdb - -rm -r $d diff --git a/virt-p2v.pod b/virt-p2v.pod index 5f4aa5763524..eb220cbf1cee 100644 --- a/virt-p2v.pod +++ b/virt-p2v.pod @@ -509,19 +509,10 @@ 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=qemu-nbd,qemu-nbd-no-sa,nbdkit,nbdkit-no-sa> +checked and the first one found is used: I<--nbd=nbdkit,nbdkit-no-sa> =over 4 -=item B<qemu-nbd> - -Use qemu-nbd. - -=item B<qemu-nbd-no-sa> - -Use qemu-nbd, but disable socket activation. - =item B<nbdkit> Use nbdkit with the file plugin (see: L<nbdkit-file-plugin(1)>). @@ -532,9 +523,9 @@ Use nbdkit, but disable socket activation =back -The C<*-no-sa> variants allow virt-p2v to fall back to older versions -of qemu-nbd and nbdkit which did not support -L<socket activation|http://0pointer.de/blog/projects/socket-activation.html>. +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> @@ -679,22 +670,21 @@ 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<qemu-nbd(1)> by -default but others can be selected using the I<--nbd> command line -option. +which is proxied over ssh. The NBD server is L<nbdkit(1)>; socket +activation can be controlled using the I<--nbd> command line option. There is one ssh connection per physical hard disk on the source machine (the common case ? a single hard disk ? is shown below): ???????????????? ??????????????????? ? virt-p2v ? ? virt-v2v ? ? (physical ? control connection ? (conversion ? ? server) ???????????????????????????? server) ? ? ? ? ? ? ? data connection ? ? ? ???????????????????????????? ? - ?qemu-nbd ? ?? ? ??? ? NBD ? - ?/dev/sda ? ? requests ? + ? nbdkit ? ?? ? ??? ? NBD ? + ? /dev/sda ? ? requests ? ? ? ? ? ???????????????? ??????????????????? @@ -708,7 +698,7 @@ that virt-v2v via libguestfs can open nbd connections which directly read the hard disk(s) of the physical server. Two layers of protection are used to ensure that there are no writes -to the hard disks: Firstly, the qemu-nbd I<-r> (readonly) option is +to the hard disks: Firstly, the nbdkit I<-r> (readonly) option is used. Secondly libguestfs creates an overlay on top of the NBD connection which stores writes in a temporary file on the conversion file. @@ -731,7 +721,6 @@ L<virt-p2v-make-disk(1)>, L<virt-p2v-make-kickstart(1)>, L<virt-p2v-make-kiwi(1)>, L<virt-v2v(1)>, -L<qemu-nbd(1)>, L<nbdkit(1)>, L<nbdkit-file-plugin(1)>, L<ssh(1)>, L<sshd(8)>, -- 2.19.1.3.g30247aa5d201
Laszlo Ersek
2022-Mar-31 07:22 UTC
[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 at redhat.com> Signed-off-by: Laszlo Ersek <lersek at redhat.com> --- 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
Laszlo Ersek
2022-Mar-31 07:22 UTC
[Libguestfs] [p2v PATCH 03/10] nbd.c: simplify start_nbdkit()
The previous patch guarantees that start_nbdkit() is only called with a non-NULL "fds" parameter (and, consequently, that start_nbdkit() does not use "ipaddr" and "port" for anything beyond logging). Remove the branch in start_nbdkit() that starts nbdkit without socket activation, and trim the start_nbdkit() prototype too. This patch is best viewed with "git show -b" due to the unindentation in it. 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> --- nbd.c | 54 ++++++-------------- 1 file changed, 16 insertions(+), 38 deletions(-) diff --git a/nbd.c b/nbd.c index edc3ab51d889..cda962d03bdd 100644 --- a/nbd.c +++ b/nbd.c @@ -80,7 +80,7 @@ static const enum nbd_server standard_servers[] */ 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 pid_t start_nbdkit (const char *device, int *fds, size_t nr_fds); 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); @@ -238,29 +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); + pid = start_nbdkit (device, fds, nr_fds); for (i = 0; i < nr_fds; ++i) close (fds[i]); free (fds); return pid; } 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. */ @@ -268,106 +268,84 @@ static inline void socket_activation (int *fds, size_t nr_fds) { size_t i; char nr_fds_str[16]; char pid_str[16]; if (fds == NULL) return; for (i = 0; i < nr_fds; ++i) { int fd = FIRST_SOCKET_ACTIVATION_FD + i; if (fds[i] != fd) { dup2 (fds[i], fd); close (fds[i]); } } snprintf (nr_fds_str, sizeof nr_fds_str, "%zu", nr_fds); setenv ("LISTEN_FDS", nr_fds_str, 1); snprintf (pid_str, sizeof pid_str, "%d", (int) getpid ()); setenv ("LISTEN_PID", pid_str, 1); } /** * Start a local L<nbdkit(1)> process using the * L<nbdkit-file-plugin(1)>. * - * If we are using socket activation, C<fds> and C<nr_fds> will - * contain the locally pre-opened file descriptors for this. - * Otherwise if C<fds == NULL> we pass the port number. + * C<fds> and C<nr_fds> will contain the locally pre-opened file descriptors + * for this. * * Returns the process ID (E<gt> 0) or C<0> if there is an error. */ static pid_t -start_nbdkit (const char *device, - const char *ipaddr, int port, int *fds, size_t nr_fds) +start_nbdkit (const char *device, 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"); + fprintf (stderr, "starting nbdkit for %s using socket activation\n", device); #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); + 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); - } + 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 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. */ -- 2.19.1.3.g30247aa5d201
Laszlo Ersek
2022-Mar-31 07:22 UTC
[Libguestfs] [p2v PATCH 04/10] open-code "localhost" as the listen hostname for nbdkit
At this point, the only NBD server type virt-p2v supports is nbdkit with socket activation; therefore, outputting "localhost" from start_nbd_server(), and tracking it via variables / parameters from there on is needless generality. Remove the affected variables and parameters, such as "ipaddr", "hostname", and "nbd_local_ipaddr". 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> --- conversion.c | 13 ++--- nbd.c | 51 +++++++++----------- p2v.h | 6 +-- ssh.c | 6 +-- 4 files changed, 33 insertions(+), 43 deletions(-) diff --git a/conversion.c b/conversion.c index 8daa1d9f39fa..8bc4119fb47c 100644 --- a/conversion.c +++ b/conversion.c @@ -155,270 +155,267 @@ int start_conversion (struct config *config, void (*notify_ui) (int type, const char *data)) { int ret = -1; int status; size_t i, len; const size_t nr_disks = guestfs_int_count_strings (config->disks); time_t now; struct tm tm; CLEANUP_FREE struct data_conn *data_conns = NULL; CLEANUP_FREE char *remote_dir = NULL; char tmpdir[] = "/tmp/p2v.XXXXXX"; char name_file[] = "/tmp/p2v.XXXXXX/name"; char physical_xml_file[] = "/tmp/p2v.XXXXXX/physical.xml"; char wrapper_script[] = "/tmp/p2v.XXXXXX/virt-v2v-wrapper.sh"; char dmesg_file[] = "/tmp/p2v.XXXXXX/dmesg"; char lscpu_file[] = "/tmp/p2v.XXXXXX/lscpu"; char lspci_file[] = "/tmp/p2v.XXXXXX/lspci"; char lsscsi_file[] = "/tmp/p2v.XXXXXX/lsscsi"; char lsusb_file[] = "/tmp/p2v.XXXXXX/lsusb"; char p2v_version_file[] = "/tmp/p2v.XXXXXX/p2v-version"; int inhibit_fd = -1; #if DEBUG_STDERR print_config (config, stderr); fprintf (stderr, "\n"); #endif set_control_h (NULL); set_running (1); set_cancel_requested (0); inhibit_fd = inhibit_power_saving (); #ifdef DEBUG_STDERR if (inhibit_fd == -1) fprintf (stderr, "warning: virt-p2v cannot inhibit power saving during conversion.\n"); #endif data_conns = malloc (sizeof (struct data_conn) * nr_disks); if (data_conns == NULL) error (EXIT_FAILURE, errno, "malloc"); for (i = 0; config->disks[i] != NULL; ++i) { data_conns[i].h = NULL; data_conns[i].nbd_pid = 0; data_conns[i].nbd_remote_port = -1; } /* Start the data connections and NBD server processes, one per disk. */ for (i = 0; config->disks[i] != NULL; ++i) { - const char *nbd_local_ipaddr; int nbd_local_port; CLEANUP_FREE char *device = NULL; if (config->disks[i][0] == '/') { device = strdup (config->disks[i]); if (!device) { perror ("strdup"); cleanup_data_conns (data_conns, nr_disks); exit (EXIT_FAILURE); } } else if (asprintf (&device, "/dev/%s", config->disks[i]) == -1) { perror ("asprintf"); cleanup_data_conns (data_conns, nr_disks); exit (EXIT_FAILURE); } if (notify_ui) { CLEANUP_FREE char *msg; if (asprintf (&msg, _("Starting local NBD server for %s ..."), config->disks[i]) == -1) error (EXIT_FAILURE, errno, "asprintf"); notify_ui (NOTIFY_STATUS, msg); } /* Start NBD server listening on the given port number. */ - data_conns[i].nbd_pid - start_nbd_server (&nbd_local_ipaddr, &nbd_local_port, device); + data_conns[i].nbd_pid = start_nbd_server (&nbd_local_port, device); if (data_conns[i].nbd_pid == 0) { set_conversion_error ("NBD server error: %s", get_nbd_error ()); goto out; } /* Wait for NBD server to start up and listen. */ - if (wait_for_nbd_server_to_start (nbd_local_ipaddr, nbd_local_port) == -1) { + if (wait_for_nbd_server_to_start (nbd_local_port) == -1) { set_conversion_error ("NBD server error: %s", get_nbd_error ()); goto out; } if (notify_ui) { CLEANUP_FREE char *msg; if (asprintf (&msg, _("Opening data connection for %s ..."), config->disks[i]) == -1) error (EXIT_FAILURE, errno, "asprintf"); notify_ui (NOTIFY_STATUS, msg); } /* Open the SSH data connection, with reverse port forwarding * back to the NBD server. */ - data_conns[i].h = open_data_connection (config, - nbd_local_ipaddr, nbd_local_port, + data_conns[i].h = open_data_connection (config, nbd_local_port, &data_conns[i].nbd_remote_port); if (data_conns[i].h == NULL) { const char *err = get_ssh_error (); set_conversion_error ("could not open data connection over SSH to the conversion server: %s", err); goto out; } #if DEBUG_STDERR fprintf (stderr, - "%s: data connection for %s: SSH remote port %d, local port %s:%d\n", + "%s: data connection for %s: SSH remote port %d, local port %d\n", g_get_prgname (), device, data_conns[i].nbd_remote_port, - nbd_local_ipaddr, nbd_local_port); + nbd_local_port); #endif } /* Create a remote directory name which will be used for libvirt * XML, log files and other stuff. We don't delete this directory * after the run because (a) it's useful for debugging and (b) it * only contains small files. * * NB: This path MUST NOT require shell quoting. */ time (&now); gmtime_r (&now, &tm); if (asprintf (&remote_dir, "/tmp/virt-p2v-%04d%02d%02d-XXXXXXXX", tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday) == -1) { perror ("asprintf"); cleanup_data_conns (data_conns, nr_disks); exit (EXIT_FAILURE); } len = strlen (remote_dir); guestfs_int_random_string (&remote_dir[len-8], 8); if (notify_ui) notify_ui (NOTIFY_LOG_DIR, remote_dir); /* Generate the local temporary directory. */ if (mkdtemp (tmpdir) == NULL) { perror ("mkdtemp"); cleanup_data_conns (data_conns, nr_disks); exit (EXIT_FAILURE); } memcpy (name_file, tmpdir, strlen (tmpdir)); memcpy (physical_xml_file, tmpdir, strlen (tmpdir)); memcpy (wrapper_script, tmpdir, strlen (tmpdir)); memcpy (dmesg_file, tmpdir, strlen (tmpdir)); memcpy (lscpu_file, tmpdir, strlen (tmpdir)); memcpy (lspci_file, tmpdir, strlen (tmpdir)); memcpy (lsscsi_file, tmpdir, strlen (tmpdir)); memcpy (lsusb_file, tmpdir, strlen (tmpdir)); memcpy (p2v_version_file, tmpdir, strlen (tmpdir)); /* Generate the static files. */ generate_name (config, name_file); generate_physical_xml (config, data_conns, physical_xml_file); generate_wrapper_script (config, remote_dir, wrapper_script); generate_system_data (dmesg_file, lscpu_file, lspci_file, lsscsi_file, lsusb_file); generate_p2v_version_file (p2v_version_file); /* Open the control connection. This also creates remote_dir. */ if (notify_ui) notify_ui (NOTIFY_STATUS, _("Setting up the control connection ...")); set_control_h (start_remote_connection (config, remote_dir)); if (control_h == NULL) { set_conversion_error ("could not open control connection over SSH to the conversion server: %s", get_ssh_error ()); goto out; } /* Copy the static files to the remote dir. */ /* These three files must not fail, so check for errors here. */ if (scp_file (config, remote_dir, name_file, physical_xml_file, wrapper_script, NULL) == -1) { set_conversion_error ("scp: %s: %s", remote_dir, get_ssh_error ()); goto out; } /* It's not essential that these files are copied, so ignore errors. */ ignore_value (scp_file (config, remote_dir, dmesg_file, lscpu_file, lspci_file, lsscsi_file, lsusb_file, p2v_version_file, NULL)); /* Do the conversion. This runs until virt-v2v exits. */ if (notify_ui) notify_ui (NOTIFY_STATUS, _("Doing conversion ...")); if (mexp_printf (control_h, /* To simplify things in the wrapper script, it * writes virt-v2v's exit status to * /remote_dir/status, and here we read that and * exit the ssh shell with the same status. */ "%s/virt-v2v-wrapper.sh; " "exit $(< %s/status)\n", remote_dir, remote_dir) == -1) { set_conversion_error ("mexp_printf: virt-v2v: %m"); goto out; } /* Read output from the virt-v2v process and echo it through the * notify function, until virt-v2v closes the connection. */ while (!is_cancel_requested ()) { char buf[257]; ssize_t r; r = read (mexp_get_fd (control_h), buf, sizeof buf - 1); if (r == -1) { /* See comment about this in miniexpect.c. */ if (errno == EIO) break; /* EOF */ set_conversion_error ("read: %m"); goto out; } if (r == 0) break; /* EOF */ buf[r] = '\0'; if (notify_ui) notify_ui (NOTIFY_REMOTE_MESSAGE, buf); } if (is_cancel_requested ()) { set_conversion_error ("cancelled by user"); if (notify_ui) notify_ui (NOTIFY_STATUS, _("Conversion cancelled by user.")); goto out; } if (notify_ui) notify_ui (NOTIFY_STATUS, _("Control connection closed by remote.")); ret = 0; out: if (control_h) { mexp_h *h = control_h; set_control_h (NULL); status = mexp_close (h); if (status == -1) { set_conversion_error ("mexp_close: %m"); ret = -1; } else if (ret == 0 && WIFEXITED (status) && WEXITSTATUS (status) != 0) { set_conversion_error ("virt-v2v exited with status %d", WEXITSTATUS (status)); ret = -1; } } cleanup_data_conns (data_conns, nr_disks); if (inhibit_fd >= 0) close (inhibit_fd); set_running (0); return ret; } diff --git a/nbd.c b/nbd.c index cda962d03bdd..cb849e75dc84 100644 --- a/nbd.c +++ b/nbd.c @@ -81,9 +81,9 @@ static const enum nbd_server standard_servers[] static enum nbd_server use_server; static pid_t start_nbdkit (const char *device, int *fds, size_t nr_fds); -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); +static int open_listening_socket (int **fds, size_t *nr_fds); +static int bind_tcpip_socket (const char *port, int **fds, size_t *nr_fds); +static int connect_with_source_port (int dest_port, int source_port); static int bind_source_port (int sockfd, int family, int source_port); static char *nbd_error; @@ -235,32 +235,31 @@ test_nbd_servers (void) * Returns the process ID (E<gt> 0) or C<0> if there is an error. */ pid_t -start_nbd_server (const char **ipaddr, int *port, const char *device) +start_nbd_server (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); + *port = open_listening_socket (&fds, &nr_fds); if (*port == -1) return -1; pid = start_nbdkit (device, fds, nr_fds); for (i = 0; i < nr_fds; ++i) close (fds[i]); free (fds); return pid; } 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. */ @@ -350,261 +349,257 @@ start_nbdkit (const char *device, int *fds, size_t nr_fds) * The caller must free the array. */ static int -open_listening_socket (const char *ipaddr, int **fds, size_t *nr_fds) +open_listening_socket (int **fds, size_t *nr_fds) { int port; char port_str[16]; /* This just ensures we don't try the port we previously bound to. */ port = nbd_local_port; /* Search for a free port. */ for (; port < 60000; ++port) { snprintf (port_str, sizeof port_str, "%d", port); - if (bind_tcpip_socket (ipaddr, port_str, fds, nr_fds) == 0) { + if (bind_tcpip_socket (port_str, fds, nr_fds) == 0) { /* See above. */ nbd_local_port = port + 1; return port; } } set_nbd_error ("cannot find a free local port"); return -1; } static int -bind_tcpip_socket (const char *ipaddr, const char *port, - int **fds_rtn, size_t *nr_fds_rtn) +bind_tcpip_socket (const char *port, int **fds_rtn, size_t *nr_fds_rtn) { struct addrinfo *ai = NULL; struct addrinfo hints; struct addrinfo *a; int err; int *fds = NULL; size_t nr_fds; int addr_in_use = 0; memset (&hints, 0, sizeof hints); hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; hints.ai_socktype = SOCK_STREAM; - err = getaddrinfo (ipaddr, port, &hints, &ai); + err = getaddrinfo ("localhost", port, &hints, &ai); if (err != 0) { #if DEBUG_STDERR - fprintf (stderr, "%s: getaddrinfo: %s: %s: %s", - g_get_prgname (), ipaddr ? ipaddr : "<any>", port, - gai_strerror (err)); + fprintf (stderr, "%s: getaddrinfo: localhost: %s: %s", g_get_prgname (), + port, gai_strerror (err)); #endif return -1; } nr_fds = 0; for (a = ai; a != NULL; a = a->ai_next) { int sock, opt; sock = socket (a->ai_family, a->ai_socktype, a->ai_protocol); if (sock == -1) error (EXIT_FAILURE, errno, "socket"); opt = 1; if (setsockopt (sock, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof opt) == -1) perror ("setsockopt: SO_REUSEADDR"); #ifdef IPV6_V6ONLY if (a->ai_family == PF_INET6) { if (setsockopt (sock, IPPROTO_IPV6, IPV6_V6ONLY, &opt, sizeof opt) == -1) perror ("setsockopt: IPv6 only"); } #endif if (bind (sock, a->ai_addr, a->ai_addrlen) == -1) { if (errno == EADDRINUSE) { addr_in_use = 1; close (sock); continue; } perror ("bind"); close (sock); continue; } if (listen (sock, SOMAXCONN) == -1) { perror ("listen"); close (sock); continue; } nr_fds++; fds = realloc (fds, sizeof (int) * nr_fds); if (!fds) error (EXIT_FAILURE, errno, "realloc"); fds[nr_fds-1] = sock; } freeaddrinfo (ai); if (nr_fds == 0 && addr_in_use) { #if DEBUG_STDERR - fprintf (stderr, "%s: unable to bind to %s:%s: %s\n", - g_get_prgname (), ipaddr ? ipaddr : "<any>", port, - strerror (EADDRINUSE)); + fprintf (stderr, "%s: unable to bind to localhost:%s: %s\n", + g_get_prgname (), port, strerror (EADDRINUSE)); #endif return -1; } #if DEBUG_STDERR - fprintf (stderr, "%s: bound to IP address %s:%s (%zu socket(s))\n", - g_get_prgname (), ipaddr ? ipaddr : "<any>", port, nr_fds); + fprintf (stderr, "%s: bound to localhost:%s (%zu socket(s))\n", + g_get_prgname (), port, nr_fds); #endif *fds_rtn = fds; *nr_fds_rtn = nr_fds; return 0; } /** * Wait for a local NBD server to start and be listening for * connections. */ int -wait_for_nbd_server_to_start (const char *ipaddr, int port) +wait_for_nbd_server_to_start (int port) { int sockfd = -1; int result = -1; time_t start_t, now_t; struct timespec half_sec = { .tv_sec = 0, .tv_nsec = 500000000 }; struct timeval timeout = { .tv_usec = 0 }; char magic[8]; /* NBDMAGIC */ size_t bytes_read = 0; ssize_t recvd; time (&start_t); for (;;) { time (&now_t); if (now_t - start_t >= WAIT_NBD_TIMEOUT) { set_nbd_error ("timed out waiting for NBD server to start"); goto cleanup; } /* Source port for probing NBD server should be one greater than * port. It's not guaranteed to always bind to this port, but it * will hint the kernel to start there and try incrementally * higher ports if needed. This avoids the case where the kernel * selects port as our source port, and we immediately connect to * ourself. See: * https://bugzilla.redhat.com/show_bug.cgi?id=1167774#c9 */ - sockfd = connect_with_source_port (ipaddr, port, port+1); + sockfd = connect_with_source_port (port, port+1); if (sockfd >= 0) break; nanosleep (&half_sec, NULL); } time (&now_t); timeout.tv_sec = (start_t + WAIT_NBD_TIMEOUT) - now_t; if (setsockopt (sockfd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof timeout) == -1) { set_nbd_error ("waiting for NBD server to start: " "setsockopt(SO_RCVTIMEO): %m"); goto cleanup; } do { recvd = recv (sockfd, magic, sizeof magic - bytes_read, 0); if (recvd == -1) { set_nbd_error ("waiting for NBD server to start: recv: %m"); goto cleanup; } bytes_read += recvd; } while (bytes_read < sizeof magic); if (memcmp (magic, "NBDMAGIC", sizeof magic) != 0) { set_nbd_error ("waiting for NBD server to start: " "'NBDMAGIC' was not received from NBD server"); goto cleanup; } result = 0; cleanup: if (sockfd >= 0) close (sockfd); return result; } /** - * Connect to C<hostname:dest_port>, resolving the address using + * Connect to C<localhost:dest_port>, resolving the address using * L<getaddrinfo(3)>. * * This also sets the source port of the connection to the first free * port number E<ge> C<source_port>. * * This may involve multiple connections - to IPv4 and IPv6 for * instance. */ static int -connect_with_source_port (const char *hostname, int dest_port, int source_port) +connect_with_source_port (int dest_port, int source_port) { struct addrinfo hints; struct addrinfo *results, *rp; char dest_port_str[16]; int r, sockfd = -1; int reuseaddr = 1; snprintf (dest_port_str, sizeof dest_port_str, "%d", dest_port); memset (&hints, 0, sizeof hints); hints.ai_family = AF_UNSPEC; /* allow IPv4 or IPv6 */ hints.ai_socktype = SOCK_STREAM; hints.ai_flags = AI_NUMERICSERV; /* numeric dest port number */ hints.ai_protocol = 0; /* any protocol */ - r = getaddrinfo (hostname, dest_port_str, &hints, &results); + r = getaddrinfo ("localhost", dest_port_str, &hints, &results); if (r != 0) { - set_nbd_error ("getaddrinfo: %s/%s: %s", - hostname, dest_port_str, gai_strerror (r)); + set_nbd_error ("getaddrinfo: localhost/%s: %s", + dest_port_str, gai_strerror (r)); return -1; } for (rp = results; rp != NULL; rp = rp->ai_next) { sockfd = socket (rp->ai_family, rp->ai_socktype, rp->ai_protocol); if (sockfd == -1) continue; /* If we run p2v repeatedly (say, running the tests in a loop), * there's a decent chance we'll end up trying to bind() to a port * that is in TIME_WAIT from a prior run. Handle that gracefully * with SO_REUSEADDR. */ if (setsockopt (sockfd, SOL_SOCKET, SO_REUSEADDR, &reuseaddr, sizeof reuseaddr) == -1) perror ("warning: setsockopt"); /* Need to bind the source port. */ if (bind_source_port (sockfd, rp->ai_family, source_port) == -1) { close (sockfd); sockfd = -1; continue; } /* Connect. */ if (connect (sockfd, rp->ai_addr, rp->ai_addrlen) == -1) { set_nbd_error ("waiting for NBD server to start: " - "connect to %s/%s: %m", - hostname, dest_port_str); + "connect to localhost/%s: %m", dest_port_str); close (sockfd); sockfd = -1; continue; } break; } freeaddrinfo (results); return sockfd; } diff --git a/p2v.h b/p2v.h index a14edc52ef74..4d2d20648467 100644 --- a/p2v.h +++ b/p2v.h @@ -105,7 +105,7 @@ extern int inhibit_power_saving (void); /* ssh.c */ extern int test_connection (struct config *); -extern mexp_h *open_data_connection (struct config *, const char *local_ipaddr, int local_port, int *remote_port); +extern mexp_h *open_data_connection (struct config *, int local_port, int *remote_port); extern mexp_h *start_remote_connection (struct config *, const char *remote_dir); extern const char *get_ssh_error (void); extern int scp_file (struct config *config, const char *target, const char *local, ...) __attribute__((sentinel)); @@ -113,8 +113,8 @@ extern int scp_file (struct config *config, const char *target, const char *loca /* nbd.c */ extern void set_nbd_option (const char *opt); extern void test_nbd_servers (void); -extern pid_t start_nbd_server (const char **ipaddr, int *port, const char *device); -extern int wait_for_nbd_server_to_start (const char *ipaddr, int port); +extern pid_t start_nbd_server (int *port, const char *device); +extern int wait_for_nbd_server_to_start (int port); const char *get_nbd_error (void); /* utils.c */ diff --git a/ssh.c b/ssh.c index 42701433f0cc..5e1d60b8a2e7 100644 --- a/ssh.c +++ b/ssh.c @@ -1062,69 +1062,67 @@ compatible_version (const char *v2v_version) } mexp_h * -open_data_connection (struct config *config, - const char *local_ipaddr, int local_port, - int *remote_port) +open_data_connection (struct config *config, int local_port, int *remote_port) { mexp_h *h; char remote_arg[32]; const char *extra_args[] = { "-R", remote_arg, "-N", NULL }; CLEANUP_FREE char *port_str = NULL; const int ovecsize = 12; int ovector[ovecsize]; - snprintf (remote_arg, sizeof remote_arg, "0:%s:%d", local_ipaddr, local_port); + snprintf (remote_arg, sizeof remote_arg, "0:localhost:%d", local_port); h = start_ssh (0, config, (char **) extra_args, 0); if (h == NULL) return NULL; switch (mexp_expect (h, (mexp_regexp[]) { { 100, .re = portfwd_re }, { 0 } }, ovector, ovecsize)) { case 100: /* Ephemeral port. */ port_str = strndup (&h->buffer[ovector[2]], ovector[3]-ovector[2]); if (port_str == NULL) { set_ssh_internal_error ("strndup: %m"); mexp_close (h); return NULL; } if (sscanf (port_str, "%d", remote_port) != 1) { set_ssh_internal_error ("cannot extract the port number from '%s'", port_str); mexp_close (h); return NULL; } break; case MEXP_EOF: set_ssh_unexpected_eof ("\"ssh -R\" output"); mexp_close (h); return NULL; case MEXP_TIMEOUT: set_ssh_unexpected_timeout ("\"ssh -R\" output"); mexp_close (h); return NULL; case MEXP_ERROR: set_ssh_mexp_error ("mexp_expect"); mexp_close (h); return NULL; case MEXP_PCRE_ERROR: set_ssh_pcre_error (); mexp_close (h); return NULL; } return h; } /* Wait for the prompt. */ -- 2.19.1.3.g30247aa5d201
Laszlo Ersek
2022-Mar-31 07:22 UTC
[Libguestfs] [p2v PATCH 05/10] nbd.c: remove bind_source_port()
Commit d43f482adce2 ("p2v: avoid connecting to ourself while probing qemu-nbd (RHBZ#1167774)", 2014-12-15) resolved the following race condition: start_conversion() [conversion.c] open_data_connection() [ssh.c] *local_port = nbd_local_port; nbd_local_port++; start_qemu_nbd() [conversion.c] fork() /* in the child */ /* execute "qemu-nbd ... -p port_str" */ bind() wait_qemu_nbd() [conversion.c] connect() The race was between the child process (qemu-nbd) reaching bind() and the parent process reaching connect(). In case the parent process "won", then the kernel would assign an ephemeral source port to the connecting socket from such a pool of ports from which the child process had not bound its listening socket yet. Arguably due to a kernel bug, the kernel could then assign the same port number to the local end of the connecting socket as the port number the socket was connecting to, effectively causing the parent process to connect to itself. This race no longer exists: due to the previous patches, only socket activation is possible, in which bind() is called by the parent process for the listening socket strictly before the parent process calls connect(): start_conversion() [conversion.c] start_nbd_server() [nbd.c] open_listening_socket() [nbd.c] bind_tcpip_socket() [nbd.c] bind() start_nbdkit() [nbd.c] fork() /* in the child */ socket_activation() /* execute "nbdkit", letting it * inherit the already bound socket */ wait_for_nbd_server_to_start() [nbd.c] connect_with_source_port() [nbd.c] bind_source_port() [nbd.c] bind() connect() Remove bind_source_port(). Rename connect_with_source_port() to connect_to_nbdkit(), and remove its "source_port" parameter. 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> --- nbd.c | 74 +------------------- 1 file changed, 3 insertions(+), 71 deletions(-) diff --git a/nbd.c b/nbd.c index cb849e75dc84..f64afdc53dd3 100644 --- a/nbd.c +++ b/nbd.c @@ -83,8 +83,7 @@ static enum nbd_server use_server; static pid_t start_nbdkit (const char *device, int *fds, size_t nr_fds); static int open_listening_socket (int **fds, size_t *nr_fds); static int bind_tcpip_socket (const char *port, int **fds, size_t *nr_fds); -static int connect_with_source_port (int dest_port, int source_port); -static int bind_source_port (int sockfd, int family, int source_port); +static int connect_to_nbdkit (int dest_port); static char *nbd_error; @@ -467,178 +466,111 @@ int wait_for_nbd_server_to_start (int port) { int sockfd = -1; int result = -1; time_t start_t, now_t; struct timespec half_sec = { .tv_sec = 0, .tv_nsec = 500000000 }; struct timeval timeout = { .tv_usec = 0 }; char magic[8]; /* NBDMAGIC */ size_t bytes_read = 0; ssize_t recvd; time (&start_t); for (;;) { time (&now_t); if (now_t - start_t >= WAIT_NBD_TIMEOUT) { set_nbd_error ("timed out waiting for NBD server to start"); goto cleanup; } - /* Source port for probing NBD server should be one greater than - * port. It's not guaranteed to always bind to this port, but it - * will hint the kernel to start there and try incrementally - * higher ports if needed. This avoids the case where the kernel - * selects port as our source port, and we immediately connect to - * ourself. See: - * https://bugzilla.redhat.com/show_bug.cgi?id=1167774#c9 - */ - sockfd = connect_with_source_port (port, port+1); + sockfd = connect_to_nbdkit (port); if (sockfd >= 0) break; nanosleep (&half_sec, NULL); } time (&now_t); timeout.tv_sec = (start_t + WAIT_NBD_TIMEOUT) - now_t; if (setsockopt (sockfd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof timeout) == -1) { set_nbd_error ("waiting for NBD server to start: " "setsockopt(SO_RCVTIMEO): %m"); goto cleanup; } do { recvd = recv (sockfd, magic, sizeof magic - bytes_read, 0); if (recvd == -1) { set_nbd_error ("waiting for NBD server to start: recv: %m"); goto cleanup; } bytes_read += recvd; } while (bytes_read < sizeof magic); if (memcmp (magic, "NBDMAGIC", sizeof magic) != 0) { set_nbd_error ("waiting for NBD server to start: " "'NBDMAGIC' was not received from NBD server"); goto cleanup; } result = 0; cleanup: if (sockfd >= 0) close (sockfd); return result; } /** * Connect to C<localhost:dest_port>, resolving the address using * L<getaddrinfo(3)>. * - * This also sets the source port of the connection to the first free - * port number E<ge> C<source_port>. - * * This may involve multiple connections - to IPv4 and IPv6 for * instance. */ static int -connect_with_source_port (int dest_port, int source_port) +connect_to_nbdkit (int dest_port) { struct addrinfo hints; struct addrinfo *results, *rp; char dest_port_str[16]; int r, sockfd = -1; - int reuseaddr = 1; snprintf (dest_port_str, sizeof dest_port_str, "%d", dest_port); memset (&hints, 0, sizeof hints); hints.ai_family = AF_UNSPEC; /* allow IPv4 or IPv6 */ hints.ai_socktype = SOCK_STREAM; hints.ai_flags = AI_NUMERICSERV; /* numeric dest port number */ hints.ai_protocol = 0; /* any protocol */ r = getaddrinfo ("localhost", dest_port_str, &hints, &results); if (r != 0) { set_nbd_error ("getaddrinfo: localhost/%s: %s", dest_port_str, gai_strerror (r)); return -1; } for (rp = results; rp != NULL; rp = rp->ai_next) { sockfd = socket (rp->ai_family, rp->ai_socktype, rp->ai_protocol); if (sockfd == -1) continue; - /* If we run p2v repeatedly (say, running the tests in a loop), - * there's a decent chance we'll end up trying to bind() to a port - * that is in TIME_WAIT from a prior run. Handle that gracefully - * with SO_REUSEADDR. - */ - if (setsockopt (sockfd, SOL_SOCKET, SO_REUSEADDR, - &reuseaddr, sizeof reuseaddr) == -1) - perror ("warning: setsockopt"); - - /* Need to bind the source port. */ - if (bind_source_port (sockfd, rp->ai_family, source_port) == -1) { - close (sockfd); - sockfd = -1; - continue; - } - /* Connect. */ if (connect (sockfd, rp->ai_addr, rp->ai_addrlen) == -1) { set_nbd_error ("waiting for NBD server to start: " "connect to localhost/%s: %m", dest_port_str); close (sockfd); sockfd = -1; continue; } break; } freeaddrinfo (results); return sockfd; } - -static int -bind_source_port (int sockfd, int family, int source_port) -{ - struct addrinfo hints; - struct addrinfo *results, *rp; - char source_port_str[16]; - int r; - - snprintf (source_port_str, sizeof source_port_str, "%d", source_port); - - memset (&hints, 0, sizeof (hints)); - hints.ai_family = family; - hints.ai_socktype = SOCK_STREAM; - hints.ai_flags = AI_PASSIVE | AI_NUMERICSERV; /* numeric port number */ - hints.ai_protocol = 0; /* any protocol */ - - r = getaddrinfo ("localhost", source_port_str, &hints, &results); - if (r != 0) { - set_nbd_error ("getaddrinfo (bind): localhost/%s: %s", - source_port_str, gai_strerror (r)); - return -1; - } - - for (rp = results; rp != NULL; rp = rp->ai_next) { - if (bind (sockfd, rp->ai_addr, rp->ai_addrlen) == 0) - goto bound; - } - - set_nbd_error ("waiting for NBD server to start: " - "bind to source port %d: %m", - source_port); - freeaddrinfo (results); - return -1; - - bound: - freeaddrinfo (results); - return 0; -} -- 2.19.1.3.g30247aa5d201
Laszlo Ersek
2022-Mar-31 07:22 UTC
[Libguestfs] [p2v PATCH 06/10] remove "--nbd" option parsing
Remove the --nbd command line option, its documentation, the parser function set_nbd_option(), and the internal representation "cmdline_servers". Unconditionally use the "standard_servers" array (which now consists of the NBDKIT server type only). Further patches will simplify the remaining code. 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> --- main.c | 5 -- nbd.c | 64 +++----------------- p2v.h | 1 - test-virt-p2v-nbdkit.sh | 5 +- virt-p2v.pod | 14 ----- 5 files changed, 10 insertions(+), 79 deletions(-) diff --git a/main.c b/main.c index e4e9c1fe21c1..dc411b733d31 100644 --- a/main.c +++ b/main.c @@ -69,16 +69,15 @@ static const char options[] = "Vv"; static const struct option long_options[] = { { "help", 0, 0, HELP_OPTION }, { "cmdline", 1, 0, 0 }, { "color", 0, 0, 0 }, { "colors", 0, 0, 0 }, { "colour", 0, 0, 0 }, { "colours", 0, 0, 0 }, { "iso", 0, 0, 0 }, - { "nbd", 1, 0, 0 }, { "long-options", 0, 0, 0 }, { "short-options", 0, 0, 0 }, { "test-disk", 1, 0, 0 }, { "verbose", 0, 0, 'v' }, { "version", 0, 0, 'V' }, { 0, 0, 0, 0 } }; @@ -87,27 +86,26 @@ static void __attribute__((noreturn)) usage (int status) { if (status != EXIT_SUCCESS) fprintf (stderr, _("Try ?%s --help? for more information.\n"), g_get_prgname ()); else { printf (_("%s: Convert a physical machine to use KVM\n" "Copyright (C) 2009-2019 Red Hat Inc.\n" "Usage:\n" " %s [--options]\n" "Options:\n" " --help Display brief help\n" " --cmdline=CMDLINE Used to debug command line parsing\n" " --colors|--colours Use ANSI colour sequences even if not tty\n" " --iso Running in the ISO environment\n" - " --nbd=nbdkit Search order for NBD servers\n" " --test-disk=DISK.IMG For testing, use disk as /dev/sda\n" " -v|--verbose Verbose messages\n" " -V|--version Display version and exit\n" "For more information, see the manpage %s(1).\n"), g_get_prgname (), g_get_prgname (), g_get_prgname ()); } exit (status); } /* XXX Copied from fish/options.c. */ @@ -137,128 +135,125 @@ int main (int argc, char *argv[]) { gboolean gui_possible; int c; int option_index; char **cmdline = NULL; int cmdline_source = 0; struct config *config = new_config (); setlocale (LC_ALL, ""); bindtextdomain (PACKAGE, LOCALEBASEDIR); textdomain (PACKAGE); /* We may use random(3) in this program. */ srandom (time (NULL) + getpid ()); /* There is some raciness between slow devices being discovered by * the kernel and udev and virt-p2v running. This is a partial * workaround, but a real fix involves handling hotplug events * (possible in GUI mode, not easy in kernel mode). */ udevadm_settle (); gui_possible = gtk_init_check (&argc, &argv); for (;;) { c = getopt_long (argc, argv, options, long_options, &option_index); if (c == -1) break; switch (c) { case 0: /* options which are long only */ if (STREQ (long_options[option_index].name, "long-options")) { display_long_options (long_options); } else if (STREQ (long_options[option_index].name, "short-options")) { display_short_options (options); } else if (STREQ (long_options[option_index].name, "cmdline")) { cmdline = parse_cmdline_string (optarg); cmdline_source = CMDLINE_SOURCE_COMMAND_LINE; } else if (STREQ (long_options[option_index].name, "color") || STREQ (long_options[option_index].name, "colour") || STREQ (long_options[option_index].name, "colors") || STREQ (long_options[option_index].name, "colours")) { force_colour = 1; } else if (STREQ (long_options[option_index].name, "iso")) { is_iso_environment = 1; } - else if (STREQ (long_options[option_index].name, "nbd")) { - set_nbd_option (optarg); /* in nbd.c */ - } else if (STREQ (long_options[option_index].name, "test-disk")) { if (test_disk != NULL) error (EXIT_FAILURE, 0, _("only a single --test-disk option can be used")); if (optarg[0] != '/') error (EXIT_FAILURE, 0, _("--test-disk must be an absolute path")); test_disk = optarg; } else error (EXIT_FAILURE, 0, _("unknown long option: %s (%d)"), long_options[option_index].name, option_index); break; case 'v': /* This option does nothing since 1.33.41. Verbose is always * enabled. */ break; case 'V': printf ("%s %s\n", g_get_prgname (), PACKAGE_VERSION_FULL); exit (EXIT_SUCCESS); case HELP_OPTION: usage (EXIT_SUCCESS); default: usage (EXIT_FAILURE); } } if (optind != argc) { fprintf (stderr, _("%s: unused arguments on the command line\n"), g_get_prgname ()); usage (EXIT_FAILURE); } test_nbd_servers (); set_config_defaults (config); /* Parse /proc/cmdline (if it exists) or use the --cmdline parameter * to initialize the configuration. This allows defaults to be pass * using the kernel command line, with additional GUI configuration * later. */ if (cmdline == NULL) { cmdline = parse_proc_cmdline (); if (cmdline != NULL) cmdline_source = CMDLINE_SOURCE_PROC_CMDLINE; } if (cmdline) update_config_from_kernel_cmdline (config, cmdline); /* If p2v.server exists, then we use the non-interactive kernel * conversion. Otherwise we run the GUI. */ if (config->remote.server != NULL) kernel_conversion (config, cmdline, cmdline_source); else { if (!gui_possible) error (EXIT_FAILURE, 0, _("gtk_init_check returned false, indicating that\n" "a GUI is not possible on this host. Check X11, $DISPLAY etc.")); gui_conversion (config); } guestfs_int_free_string_list (cmdline); free_config (config); exit (EXIT_SUCCESS); } diff --git a/nbd.c b/nbd.c index f64afdc53dd3..7d8abf3cbbb0 100644 --- a/nbd.c +++ b/nbd.c @@ -1,76 +1,72 @@ /* virt-p2v * Copyright (C) 2009-2019 Red Hat Inc. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation; either version 2 of the License, or * (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program. If not, see <https://www.gnu.org/licenses/>. */ -/** - * This file handles the virt-p2v I<--nbd> command line option - * and running L<nbdkit(1)>. - */ +/* This file handles running L<nbdkit(1)>. */ #include <config.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <inttypes.h> #include <fcntl.h> #include <unistd.h> #include <time.h> #include <netdb.h> #include <errno.h> #include <error.h> #include <libintl.h> #include <sys/types.h> #include <sys/socket.h> #include <assert.h> #include "p2v.h" /* How long to wait for the NBD server to start (seconds). */ #define WAIT_NBD_TIMEOUT 10 /* The local port that the NBD server listens on (incremented for * each server which is started). */ static int nbd_local_port; -/* List of servers specified by the --nbd option. */ +/* Supported server types. */ enum nbd_server { /* 0 is reserved for "end of list" */ NBDKIT = 1, }; -static enum nbd_server *cmdline_servers = NULL; static const char * nbd_server_string (enum nbd_server s) { const char *ret = NULL; switch (s) { case NBDKIT: ret = "nbdkit"; 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). +/* We use this standard list of nbd server types. Must match the documentation + * in virt-p2v(1). */ static const enum nbd_server standard_servers[] { NBDKIT, 0 }; @@ -113,47 +109,11 @@ const char * get_nbd_error (void) { return nbd_error; } /** - * The main program calls this to set the I<--nbd> option. - */ -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 - 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. + * Test the built-in default list to see which servers are actually installed + * and appear to be working. * * Set the C<use_server> global accordingly. */ @@ -161,75 +121,67 @@ 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; + 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 ); 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"), + _("%s: no working NBD server was found, cannot continue.\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. */ diff --git a/p2v.h b/p2v.h index 4d2d20648467..3093e64ca3d7 100644 --- a/p2v.h +++ b/p2v.h @@ -111,7 +111,6 @@ extern const char *get_ssh_error (void); extern int scp_file (struct config *config, const char *target, const char *local, ...) __attribute__((sentinel)); /* nbd.c */ -extern void set_nbd_option (const char *opt); extern void test_nbd_servers (void); extern pid_t start_nbd_server (int *port, const char *device); extern int wait_for_nbd_server_to_start (int port); diff --git a/test-virt-p2v-nbdkit.sh b/test-virt-p2v-nbdkit.sh index 99bb8d9fe293..68696f06605f 100755 --- a/test-virt-p2v-nbdkit.sh +++ b/test-virt-p2v-nbdkit.sh @@ -1,21 +1,21 @@ #!/bin/bash - # libguestfs virt-p2v test script # Copyright (C) 2014-2019 Red Hat Inc. # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by # the Free Software Foundation; either version 2 of the License, or # (at your option) any later version. # # This program is distributed in the hope that it will be useful, # but WITHOUT ANY WARRANTY; without even the implied warranty of # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU General Public License for more details. # # You should have received a copy of the GNU General Public License # along with this program. If not, see <https://www.gnu.org/licenses/>. -# Test virt-p2v in non-GUI mode using nbdkit. +# Test virt-p2v in non-GUI mode. set -e @@ -46,8 +46,7 @@ export PATH=$d:$PATH # The Linux kernel command line. 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 +$VG virt-p2v --cmdline="$cmdline" # 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 e6cbb1514edc..09d3c7729e38 100644 --- a/virt-p2v.pod +++ b/virt-p2v.pod @@ -506,20 +506,6 @@ virt-p2v ISO environment, ie. when it is running on a real physical machine (and thus not when testing). It enables various dangerous 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>. - -=over 4 - -=item B<nbdkit> - -Use nbdkit with the file plugin (see: L<nbdkit-file-plugin(1)>) and -socket activation. - -=back - =item B<--test-disk=/PATH/TO/DISK.IMG> For testing or debugging purposes, replace F</dev/sda> with a local -- 2.19.1.3.g30247aa5d201
Laszlo Ersek
2022-Mar-31 07:22 UTC
[Libguestfs] [p2v PATCH 07/10] nbd.c: remove nbd_server_string()
Replace nbd_server_string() with its only possible output "nbdkit". 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> --- nbd.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/nbd.c b/nbd.c index 7d8abf3cbbb0..48926d4aec1d 100644 --- a/nbd.c +++ b/nbd.c @@ -50,21 +50,6 @@ enum nbd_server { NBDKIT = 1, }; -static const char * -nbd_server_string (enum nbd_server s) -{ - const char *ret = NULL; - - switch (s) { - case NBDKIT: ret = "nbdkit"; break; - } - - if (ret == NULL) - abort (); - - return ret; -} - /* We use this standard list of nbd server types. Must match the documentation * in virt-p2v(1). */ @@ -121,67 +106,67 @@ 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); 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])); + fprintf (stderr, "checking for nbdkit ...\n"); #endif switch (servers[i]) { case NBDKIT: /* with socket activation */ 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"), g_get_prgname ()); exit (EXIT_FAILURE); } #if DEBUG_STDERR - fprintf (stderr, "picked %s\n", nbd_server_string (use_server)); + fprintf (stderr, "picked nbdkit\n"); #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. */ -- 2.19.1.3.g30247aa5d201
Laszlo Ersek
2022-Mar-31 07:22 UTC
[Libguestfs] [p2v PATCH 08/10] nbd.c: remove "enum nbd_server"
Remove the "enum nbd_server" type, and the "standard_servers" and "use_server" variables, as no real selection is happening now. With regard to naming: internally to "nbd.c", and in the user-visible documentation, replace (or restrict) "NBD server" references with "nbdkit". In source code different from "nbd.c" however, stick with the "NBD server" language. Rename test_nbd_servers() (plural) to test_nbd_server() (singular). This patch is best viewed with "git show -b"; otherwise, the unindentations due to the switch statements' removal obscure the diff somewhat. 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> --- main.c | 2 +- nbd.c | 110 ++++++-------------- p2v.h | 2 +- virt-p2v.pod | 6 +- 4 files changed, 34 insertions(+), 86 deletions(-) diff --git a/main.c b/main.c index dc411b733d31..0ebb7291c7ce 100644 --- a/main.c +++ b/main.c @@ -135,125 +135,125 @@ int main (int argc, char *argv[]) { gboolean gui_possible; int c; int option_index; char **cmdline = NULL; int cmdline_source = 0; struct config *config = new_config (); setlocale (LC_ALL, ""); bindtextdomain (PACKAGE, LOCALEBASEDIR); textdomain (PACKAGE); /* We may use random(3) in this program. */ srandom (time (NULL) + getpid ()); /* There is some raciness between slow devices being discovered by * the kernel and udev and virt-p2v running. This is a partial * workaround, but a real fix involves handling hotplug events * (possible in GUI mode, not easy in kernel mode). */ udevadm_settle (); gui_possible = gtk_init_check (&argc, &argv); for (;;) { c = getopt_long (argc, argv, options, long_options, &option_index); if (c == -1) break; switch (c) { case 0: /* options which are long only */ if (STREQ (long_options[option_index].name, "long-options")) { display_long_options (long_options); } else if (STREQ (long_options[option_index].name, "short-options")) { display_short_options (options); } else if (STREQ (long_options[option_index].name, "cmdline")) { cmdline = parse_cmdline_string (optarg); cmdline_source = CMDLINE_SOURCE_COMMAND_LINE; } else if (STREQ (long_options[option_index].name, "color") || STREQ (long_options[option_index].name, "colour") || STREQ (long_options[option_index].name, "colors") || STREQ (long_options[option_index].name, "colours")) { force_colour = 1; } else if (STREQ (long_options[option_index].name, "iso")) { is_iso_environment = 1; } else if (STREQ (long_options[option_index].name, "test-disk")) { if (test_disk != NULL) error (EXIT_FAILURE, 0, _("only a single --test-disk option can be used")); if (optarg[0] != '/') error (EXIT_FAILURE, 0, _("--test-disk must be an absolute path")); test_disk = optarg; } else error (EXIT_FAILURE, 0, _("unknown long option: %s (%d)"), long_options[option_index].name, option_index); break; case 'v': /* This option does nothing since 1.33.41. Verbose is always * enabled. */ break; case 'V': printf ("%s %s\n", g_get_prgname (), PACKAGE_VERSION_FULL); exit (EXIT_SUCCESS); case HELP_OPTION: usage (EXIT_SUCCESS); default: usage (EXIT_FAILURE); } } if (optind != argc) { fprintf (stderr, _("%s: unused arguments on the command line\n"), g_get_prgname ()); usage (EXIT_FAILURE); } - test_nbd_servers (); + test_nbd_server (); set_config_defaults (config); /* Parse /proc/cmdline (if it exists) or use the --cmdline parameter * to initialize the configuration. This allows defaults to be pass * using the kernel command line, with additional GUI configuration * later. */ if (cmdline == NULL) { cmdline = parse_proc_cmdline (); if (cmdline != NULL) cmdline_source = CMDLINE_SOURCE_PROC_CMDLINE; } if (cmdline) update_config_from_kernel_cmdline (config, cmdline); /* If p2v.server exists, then we use the non-interactive kernel * conversion. Otherwise we run the GUI. */ if (config->remote.server != NULL) kernel_conversion (config, cmdline, cmdline_source); else { if (!gui_possible) error (EXIT_FAILURE, 0, _("gtk_init_check returned false, indicating that\n" "a GUI is not possible on this host. Check X11, $DISPLAY etc.")); gui_conversion (config); } guestfs_int_free_string_list (cmdline); free_config (config); exit (EXIT_SUCCESS); } diff --git a/nbd.c b/nbd.c index 48926d4aec1d..e30b4a2195cb 100644 --- a/nbd.c +++ b/nbd.c @@ -1,66 +1,49 @@ /* virt-p2v * Copyright (C) 2009-2019 Red Hat Inc. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation; either version 2 of the License, or * (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program. If not, see <https://www.gnu.org/licenses/>. */ /* This file handles running L<nbdkit(1)>. */ #include <config.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <inttypes.h> #include <fcntl.h> #include <unistd.h> #include <time.h> #include <netdb.h> #include <errno.h> #include <error.h> #include <libintl.h> #include <sys/types.h> #include <sys/socket.h> #include <assert.h> #include "p2v.h" -/* How long to wait for the NBD server to start (seconds). */ +/* How long to wait for nbdkit to start (seconds). */ #define WAIT_NBD_TIMEOUT 10 -/* The local port that the NBD server listens on (incremented for - * each server which is started). +/* The local port that nbdkit listens on (incremented for each server which is + * started). */ static int nbd_local_port; -/* Supported server types. */ -enum nbd_server { - /* 0 is reserved for "end of list" */ - NBDKIT = 1, -}; - -/* We use this standard list of nbd server types. Must match the documentation - * in virt-p2v(1). - */ -static const enum nbd_server standard_servers[] - { 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, int *fds, size_t nr_fds); static int open_listening_socket (int **fds, size_t *nr_fds); static int bind_tcpip_socket (const char *port, int **fds, size_t *nr_fds); @@ -94,79 +77,53 @@ const char * get_nbd_error (void) { return nbd_error; } /** - * Test the built-in default list to see which servers are actually installed - * and appear to be working. - * - * Set the C<use_server> global accordingly. + * Check for nbdkit. */ void -test_nbd_servers (void) +test_nbd_server (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); - servers = standard_servers; - - use_server = 0; - - for (i = 0; servers[i] != 0; ++i) { #if DEBUG_STDERR - fprintf (stderr, "checking for nbdkit ...\n"); + fprintf (stderr, "checking for nbdkit ...\n"); #endif - switch (servers[i]) { - case NBDKIT: /* with socket activation */ - r = system ("nbdkit file --version" + r = system ("nbdkit file --version" #ifndef DEBUG_STDERR - " >/dev/null 2>&1" + " >/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"), + ); + if (r != 0) { + fprintf (stderr, _("%s: nbdkit was not found, cannot continue.\n"), g_get_prgname ()); exit (EXIT_FAILURE); } #if DEBUG_STDERR - fprintf (stderr, "picked nbdkit\n"); + fprintf (stderr, "found nbdkit\n"); #endif } /** - * Start the NBD server. + * Start nbdkit. * - * We previously tested all NBD servers (see C<test_nbd_servers>) and - * hopefully found one which will work. + * We previously tested nbdkit (see C<test_nbd_server>). * * Returns the process ID (E<gt> 0) or C<0> if there is an error. */ @@ -174,28 +131,23 @@ pid_t start_nbd_server (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 */ - *port = open_listening_socket (&fds, &nr_fds); - if (*port == -1) return -1; - pid = start_nbdkit (device, fds, nr_fds); - for (i = 0; i < nr_fds; ++i) - close (fds[i]); - free (fds); - return pid; - } - - abort (); + *port = open_listening_socket (&fds, &nr_fds); + if (*port == -1) return -1; + pid = start_nbdkit (device, fds, nr_fds); + for (i = 0; i < nr_fds; ++i) + close (fds[i]); + free (fds); + return pid; } #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. */ @@ -235,52 +187,50 @@ static pid_t start_nbdkit (const char *device, int *fds, size_t nr_fds) { pid_t pid; CLEANUP_FREE char *file_str = NULL; #if DEBUG_STDERR fprintf (stderr, "starting nbdkit for %s using socket activation\n", device); #endif 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); } 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 which supports - * socket activation. We can open a listening socket on an unused - * local port and return it. + * 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. */ @@ -311,160 +261,158 @@ static int bind_tcpip_socket (const char *port, int **fds_rtn, size_t *nr_fds_rtn) { struct addrinfo *ai = NULL; struct addrinfo hints; struct addrinfo *a; int err; int *fds = NULL; size_t nr_fds; int addr_in_use = 0; memset (&hints, 0, sizeof hints); hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; hints.ai_socktype = SOCK_STREAM; err = getaddrinfo ("localhost", port, &hints, &ai); if (err != 0) { #if DEBUG_STDERR fprintf (stderr, "%s: getaddrinfo: localhost: %s: %s", g_get_prgname (), port, gai_strerror (err)); #endif return -1; } nr_fds = 0; for (a = ai; a != NULL; a = a->ai_next) { int sock, opt; sock = socket (a->ai_family, a->ai_socktype, a->ai_protocol); if (sock == -1) error (EXIT_FAILURE, errno, "socket"); opt = 1; if (setsockopt (sock, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof opt) == -1) perror ("setsockopt: SO_REUSEADDR"); #ifdef IPV6_V6ONLY if (a->ai_family == PF_INET6) { if (setsockopt (sock, IPPROTO_IPV6, IPV6_V6ONLY, &opt, sizeof opt) == -1) perror ("setsockopt: IPv6 only"); } #endif if (bind (sock, a->ai_addr, a->ai_addrlen) == -1) { if (errno == EADDRINUSE) { addr_in_use = 1; close (sock); continue; } perror ("bind"); close (sock); continue; } if (listen (sock, SOMAXCONN) == -1) { perror ("listen"); close (sock); continue; } nr_fds++; fds = realloc (fds, sizeof (int) * nr_fds); if (!fds) error (EXIT_FAILURE, errno, "realloc"); fds[nr_fds-1] = sock; } freeaddrinfo (ai); if (nr_fds == 0 && addr_in_use) { #if DEBUG_STDERR fprintf (stderr, "%s: unable to bind to localhost:%s: %s\n", g_get_prgname (), port, strerror (EADDRINUSE)); #endif return -1; } #if DEBUG_STDERR fprintf (stderr, "%s: bound to localhost:%s (%zu socket(s))\n", g_get_prgname (), port, nr_fds); #endif *fds_rtn = fds; *nr_fds_rtn = nr_fds; return 0; } /** - * Wait for a local NBD server to start and be listening for - * connections. + * Wait for nbdkit to start and be listening for connections. */ int wait_for_nbd_server_to_start (int port) { int sockfd = -1; int result = -1; time_t start_t, now_t; struct timespec half_sec = { .tv_sec = 0, .tv_nsec = 500000000 }; struct timeval timeout = { .tv_usec = 0 }; char magic[8]; /* NBDMAGIC */ size_t bytes_read = 0; ssize_t recvd; time (&start_t); for (;;) { time (&now_t); if (now_t - start_t >= WAIT_NBD_TIMEOUT) { - set_nbd_error ("timed out waiting for NBD server to start"); + set_nbd_error ("timed out waiting for nbdkit to start"); goto cleanup; } sockfd = connect_to_nbdkit (port); if (sockfd >= 0) break; nanosleep (&half_sec, NULL); } time (&now_t); timeout.tv_sec = (start_t + WAIT_NBD_TIMEOUT) - now_t; if (setsockopt (sockfd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof timeout) == -1) { - set_nbd_error ("waiting for NBD server to start: " - "setsockopt(SO_RCVTIMEO): %m"); + set_nbd_error ("waiting for nbdkit to start: setsockopt(SO_RCVTIMEO): %m"); goto cleanup; } do { recvd = recv (sockfd, magic, sizeof magic - bytes_read, 0); if (recvd == -1) { - set_nbd_error ("waiting for NBD server to start: recv: %m"); + set_nbd_error ("waiting for nbdkit to start: recv: %m"); goto cleanup; } bytes_read += recvd; } while (bytes_read < sizeof magic); if (memcmp (magic, "NBDMAGIC", sizeof magic) != 0) { - set_nbd_error ("waiting for NBD server to start: " - "'NBDMAGIC' was not received from NBD server"); + set_nbd_error ("waiting for nbdkit to start: " + "'NBDMAGIC' was not received from nbdkit"); goto cleanup; } result = 0; cleanup: if (sockfd >= 0) close (sockfd); return result; } /** * Connect to C<localhost:dest_port>, resolving the address using * L<getaddrinfo(3)>. * * This may involve multiple connections - to IPv4 and IPv6 for * instance. */ @@ -472,42 +420,42 @@ static int connect_to_nbdkit (int dest_port) { struct addrinfo hints; struct addrinfo *results, *rp; char dest_port_str[16]; int r, sockfd = -1; snprintf (dest_port_str, sizeof dest_port_str, "%d", dest_port); memset (&hints, 0, sizeof hints); hints.ai_family = AF_UNSPEC; /* allow IPv4 or IPv6 */ hints.ai_socktype = SOCK_STREAM; hints.ai_flags = AI_NUMERICSERV; /* numeric dest port number */ hints.ai_protocol = 0; /* any protocol */ r = getaddrinfo ("localhost", dest_port_str, &hints, &results); if (r != 0) { set_nbd_error ("getaddrinfo: localhost/%s: %s", dest_port_str, gai_strerror (r)); return -1; } for (rp = results; rp != NULL; rp = rp->ai_next) { sockfd = socket (rp->ai_family, rp->ai_socktype, rp->ai_protocol); if (sockfd == -1) continue; /* Connect. */ if (connect (sockfd, rp->ai_addr, rp->ai_addrlen) == -1) { - set_nbd_error ("waiting for NBD server to start: " + set_nbd_error ("waiting for nbdkit to start: " "connect to localhost/%s: %m", dest_port_str); close (sockfd); sockfd = -1; continue; } break; } freeaddrinfo (results); return sockfd; } diff --git a/p2v.h b/p2v.h index 3093e64ca3d7..c55f64317dde 100644 --- a/p2v.h +++ b/p2v.h @@ -111,7 +111,7 @@ extern const char *get_ssh_error (void); extern int scp_file (struct config *config, const char *target, const char *local, ...) __attribute__((sentinel)); /* nbd.c */ -extern void test_nbd_servers (void); +extern void test_nbd_server (void); extern pid_t start_nbd_server (int *port, const char *device); extern int wait_for_nbd_server_to_start (int port); const char *get_nbd_error (void); diff --git a/virt-p2v.pod b/virt-p2v.pod index 09d3c7729e38..aec5078f92db 100644 --- a/virt-p2v.pod +++ b/virt-p2v.pod @@ -673,9 +673,9 @@ server and terminates on the conversion server, in fact NBD requests flow in the opposite direction. This is because the reverse port forward feature of ssh (C<ssh -R>) is used to open a port on the loopback interface of the conversion server which is proxied back by -ssh to the NBD server running on the physical machine. The effect is -that virt-v2v via libguestfs can open nbd connections which directly -read the hard disk(s) of the physical server. +ssh to nbdkit running on the physical machine. The effect is that +virt-v2v via libguestfs can open nbd connections which directly read +the hard disk(s) of the physical server. Two layers of protection are used to ensure that there are no writes to the hard disks: Firstly, the nbdkit I<-r> (readonly) option is -- 2.19.1.3.g30247aa5d201
Laszlo Ersek
2022-Mar-31 07:22 UTC
[Libguestfs] [p2v PATCH 09/10] rely on Linux for killing nbdkit, when the parent thread exits
We currently track the PIDs of the NBD servers (nbdkit only, at this point) so that we can kill and reap them in cleanup_data_conns(). cleanup_data_conns() is called from three kinds of contexts: (1) in start_conversion(), followed immediately by exit (EXIT_FAILURE); (2) at the end of start_conversion() in GUI mode, where we return to start_conversion_thread(), and then the conversion thread -- a detached thread -- exits almost immediately; (3) at the end of start_conversion() in kernel mode. Here we return to kernel_conversion(), then main(), and soon exit the process. As shown above, whenever we intend to kill and reap the nbdkit processes, in any of these contexts, the parent *thread's* disappearance is also imminent. This gives rise to the idea of (a) removing the tracking & explicit killing of nbdkit processes by PID, and (b) telling the kernel to kill the nbdkit processes *upon* parent thread disappearance. And that's what the "--exit-with-parent" option of nbdkit does -- at least on Linux <https://libguestfs.org/nbdkit-captive.1.html#EXIT-WITH-PARENT> --, so replace the tracking with "--exit-with-parent". ... I have no clue how to test this patch (or how the patch affects non-Linux host OSes). 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> --- conversion.c | 12 +++--------- nbd.c | 9 +++++---- p2v.h | 1 - 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/conversion.c b/conversion.c index 8bc4119fb47c..da162d8690e3 100644 --- a/conversion.c +++ b/conversion.c @@ -155,267 +155,267 @@ int start_conversion (struct config *config, void (*notify_ui) (int type, const char *data)) { int ret = -1; int status; size_t i, len; const size_t nr_disks = guestfs_int_count_strings (config->disks); time_t now; struct tm tm; CLEANUP_FREE struct data_conn *data_conns = NULL; CLEANUP_FREE char *remote_dir = NULL; char tmpdir[] = "/tmp/p2v.XXXXXX"; char name_file[] = "/tmp/p2v.XXXXXX/name"; char physical_xml_file[] = "/tmp/p2v.XXXXXX/physical.xml"; char wrapper_script[] = "/tmp/p2v.XXXXXX/virt-v2v-wrapper.sh"; char dmesg_file[] = "/tmp/p2v.XXXXXX/dmesg"; char lscpu_file[] = "/tmp/p2v.XXXXXX/lscpu"; char lspci_file[] = "/tmp/p2v.XXXXXX/lspci"; char lsscsi_file[] = "/tmp/p2v.XXXXXX/lsscsi"; char lsusb_file[] = "/tmp/p2v.XXXXXX/lsusb"; char p2v_version_file[] = "/tmp/p2v.XXXXXX/p2v-version"; int inhibit_fd = -1; #if DEBUG_STDERR print_config (config, stderr); fprintf (stderr, "\n"); #endif set_control_h (NULL); set_running (1); set_cancel_requested (0); inhibit_fd = inhibit_power_saving (); #ifdef DEBUG_STDERR if (inhibit_fd == -1) fprintf (stderr, "warning: virt-p2v cannot inhibit power saving during conversion.\n"); #endif data_conns = malloc (sizeof (struct data_conn) * nr_disks); if (data_conns == NULL) error (EXIT_FAILURE, errno, "malloc"); for (i = 0; config->disks[i] != NULL; ++i) { data_conns[i].h = NULL; - data_conns[i].nbd_pid = 0; data_conns[i].nbd_remote_port = -1; } /* Start the data connections and NBD server processes, one per disk. */ for (i = 0; config->disks[i] != NULL; ++i) { int nbd_local_port; CLEANUP_FREE char *device = NULL; + pid_t nbd_pid; if (config->disks[i][0] == '/') { device = strdup (config->disks[i]); if (!device) { perror ("strdup"); cleanup_data_conns (data_conns, nr_disks); exit (EXIT_FAILURE); } } else if (asprintf (&device, "/dev/%s", config->disks[i]) == -1) { perror ("asprintf"); cleanup_data_conns (data_conns, nr_disks); exit (EXIT_FAILURE); } if (notify_ui) { CLEANUP_FREE char *msg; if (asprintf (&msg, _("Starting local NBD server for %s ..."), config->disks[i]) == -1) error (EXIT_FAILURE, errno, "asprintf"); notify_ui (NOTIFY_STATUS, msg); } /* Start NBD server listening on the given port number. */ - data_conns[i].nbd_pid = start_nbd_server (&nbd_local_port, device); - if (data_conns[i].nbd_pid == 0) { + nbd_pid = start_nbd_server (&nbd_local_port, device); + if (nbd_pid == 0) { set_conversion_error ("NBD server error: %s", get_nbd_error ()); goto out; } /* Wait for NBD server to start up and listen. */ if (wait_for_nbd_server_to_start (nbd_local_port) == -1) { set_conversion_error ("NBD server error: %s", get_nbd_error ()); goto out; } if (notify_ui) { CLEANUP_FREE char *msg; if (asprintf (&msg, _("Opening data connection for %s ..."), config->disks[i]) == -1) error (EXIT_FAILURE, errno, "asprintf"); notify_ui (NOTIFY_STATUS, msg); } /* Open the SSH data connection, with reverse port forwarding * back to the NBD server. */ data_conns[i].h = open_data_connection (config, nbd_local_port, &data_conns[i].nbd_remote_port); if (data_conns[i].h == NULL) { const char *err = get_ssh_error (); set_conversion_error ("could not open data connection over SSH to the conversion server: %s", err); goto out; } #if DEBUG_STDERR fprintf (stderr, "%s: data connection for %s: SSH remote port %d, local port %d\n", g_get_prgname (), device, data_conns[i].nbd_remote_port, nbd_local_port); #endif } /* Create a remote directory name which will be used for libvirt * XML, log files and other stuff. We don't delete this directory * after the run because (a) it's useful for debugging and (b) it * only contains small files. * * NB: This path MUST NOT require shell quoting. */ time (&now); gmtime_r (&now, &tm); if (asprintf (&remote_dir, "/tmp/virt-p2v-%04d%02d%02d-XXXXXXXX", tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday) == -1) { perror ("asprintf"); cleanup_data_conns (data_conns, nr_disks); exit (EXIT_FAILURE); } len = strlen (remote_dir); guestfs_int_random_string (&remote_dir[len-8], 8); if (notify_ui) notify_ui (NOTIFY_LOG_DIR, remote_dir); /* Generate the local temporary directory. */ if (mkdtemp (tmpdir) == NULL) { perror ("mkdtemp"); cleanup_data_conns (data_conns, nr_disks); exit (EXIT_FAILURE); } memcpy (name_file, tmpdir, strlen (tmpdir)); memcpy (physical_xml_file, tmpdir, strlen (tmpdir)); memcpy (wrapper_script, tmpdir, strlen (tmpdir)); memcpy (dmesg_file, tmpdir, strlen (tmpdir)); memcpy (lscpu_file, tmpdir, strlen (tmpdir)); memcpy (lspci_file, tmpdir, strlen (tmpdir)); memcpy (lsscsi_file, tmpdir, strlen (tmpdir)); memcpy (lsusb_file, tmpdir, strlen (tmpdir)); memcpy (p2v_version_file, tmpdir, strlen (tmpdir)); /* Generate the static files. */ generate_name (config, name_file); generate_physical_xml (config, data_conns, physical_xml_file); generate_wrapper_script (config, remote_dir, wrapper_script); generate_system_data (dmesg_file, lscpu_file, lspci_file, lsscsi_file, lsusb_file); generate_p2v_version_file (p2v_version_file); /* Open the control connection. This also creates remote_dir. */ if (notify_ui) notify_ui (NOTIFY_STATUS, _("Setting up the control connection ...")); set_control_h (start_remote_connection (config, remote_dir)); if (control_h == NULL) { set_conversion_error ("could not open control connection over SSH to the conversion server: %s", get_ssh_error ()); goto out; } /* Copy the static files to the remote dir. */ /* These three files must not fail, so check for errors here. */ if (scp_file (config, remote_dir, name_file, physical_xml_file, wrapper_script, NULL) == -1) { set_conversion_error ("scp: %s: %s", remote_dir, get_ssh_error ()); goto out; } /* It's not essential that these files are copied, so ignore errors. */ ignore_value (scp_file (config, remote_dir, dmesg_file, lscpu_file, lspci_file, lsscsi_file, lsusb_file, p2v_version_file, NULL)); /* Do the conversion. This runs until virt-v2v exits. */ if (notify_ui) notify_ui (NOTIFY_STATUS, _("Doing conversion ...")); if (mexp_printf (control_h, /* To simplify things in the wrapper script, it * writes virt-v2v's exit status to * /remote_dir/status, and here we read that and * exit the ssh shell with the same status. */ "%s/virt-v2v-wrapper.sh; " "exit $(< %s/status)\n", remote_dir, remote_dir) == -1) { set_conversion_error ("mexp_printf: virt-v2v: %m"); goto out; } /* Read output from the virt-v2v process and echo it through the * notify function, until virt-v2v closes the connection. */ while (!is_cancel_requested ()) { char buf[257]; ssize_t r; r = read (mexp_get_fd (control_h), buf, sizeof buf - 1); if (r == -1) { /* See comment about this in miniexpect.c. */ if (errno == EIO) break; /* EOF */ set_conversion_error ("read: %m"); goto out; } if (r == 0) break; /* EOF */ buf[r] = '\0'; if (notify_ui) notify_ui (NOTIFY_REMOTE_MESSAGE, buf); } if (is_cancel_requested ()) { set_conversion_error ("cancelled by user"); if (notify_ui) notify_ui (NOTIFY_STATUS, _("Conversion cancelled by user.")); goto out; } if (notify_ui) notify_ui (NOTIFY_STATUS, _("Control connection closed by remote.")); ret = 0; out: if (control_h) { mexp_h *h = control_h; set_control_h (NULL); status = mexp_close (h); if (status == -1) { set_conversion_error ("mexp_close: %m"); ret = -1; } else if (ret == 0 && WIFEXITED (status) && WEXITSTATUS (status) != 0) { set_conversion_error ("virt-v2v exited with status %d", WEXITSTATUS (status)); ret = -1; } } cleanup_data_conns (data_conns, nr_disks); if (inhibit_fd >= 0) close (inhibit_fd); set_running (0); return ret; } @@ -436,25 +436,19 @@ static void cleanup_data_conns (struct data_conn *data_conns, size_t nr) { size_t i; for (i = 0; i < nr; ++i) { if (data_conns[i].h != NULL) { /* Because there is no SSH prompt (ssh -N), the only way to kill * these ssh connections is to send a signal. Just closing the * pipe doesn't do anything. */ kill (mexp_get_pid (data_conns[i].h), SIGHUP); mexp_close (data_conns[i].h); } - - if (data_conns[i].nbd_pid > 0) { - /* Kill NBD process and clean up. */ - kill (data_conns[i].nbd_pid, SIGTERM); - waitpid (data_conns[i].nbd_pid, NULL, 0); - } } } /** * Write the guest name into C<filename>. */ diff --git a/nbd.c b/nbd.c index e30b4a2195cb..dcedd0a52dce 100644 --- a/nbd.c +++ b/nbd.c @@ -187,50 +187,51 @@ static pid_t start_nbdkit (const char *device, int *fds, size_t nr_fds) { pid_t pid; CLEANUP_FREE char *file_str = NULL; #if DEBUG_STDERR fprintf (stderr, "starting nbdkit for %s using socket activation\n", device); #endif 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); } 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 */ + "-r", /* readonly (vital!) */ + "--exit-with-parent", /* don't fork, and exit when the parent thread + * does */ + "file", /* file plugin */ + file_str, /* a device like file=/dev/sda */ NULL); perror ("nbdkit"); _exit (EXIT_FAILURE); } /* Parent. */ return pid; } /** * 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/p2v.h b/p2v.h index c55f64317dde..0846a2f959fa 100644 --- a/p2v.h +++ b/p2v.h @@ -85,7 +85,6 @@ extern void gui_conversion (struct config *); /* conversion.c */ struct data_conn { /* Data per NBD connection / physical disk. */ mexp_h *h; /* miniexpect handle to ssh */ - pid_t nbd_pid; /* NBD server PID */ int nbd_remote_port; /* remote NBD port on conversion server */ }; -- 2.19.1.3.g30247aa5d201
Laszlo Ersek
2022-Mar-31 07:22 UTC
[Libguestfs] [p2v PATCH 10/10] nbd.c: bind listening socket without AI_ADDRCONFIG
(This patch is similar to nbdkit commit 9eec2335d630, "server/sockets: get rid of AI_ADDRCONFIG", 2022-01-19). Consider the following call tree: start_conversion() [conversion.c] start_nbd_server() [nbd.c] open_listening_socket() [nbd.c] bind_tcpip_socket() [nbd.c] getaddrinfo() socket() bind() wait_for_nbd_server_to_start() [nbd.c] connect_to_nbdkit() [nbd.c] getaddrinfo() socket() connect() open_data_connection() [ssh.c] /* "-R 0:localhost:<port>" */ - For each of IPv4 and IPv6, if the network config on the host running virt-p2v supports that protocol, then bind_tcpip_socket() intends to bind the port for that protocol. - connect_to_nbdkit() connects to the port using *one* of IPv4 and IPv6; it just wants to see NBDMAGIC, regardless of IP version. - The ssh "-R 0:localhost:<port>" option, formatted by open_data_connection(), instructs ssh to create a reverse forwarding channel (a listening socket) per IP version (this can be verified with "netstat" on the conversion server). In case the reverse NBD connection on the conversion server were made to sshd over IPv6, then ssh on the p2v server would presumably want to connect to nbdkit over IPv6 too. The (theoretical) problem with using AI_ADDRCONFIG in bind_tcpip_socket() is that, in case the p2v server has no publicly routable IPv6 address assigned, then bind_tcpip_socket() will not bind ::1 from "localhost". And then the IPv6 reverse forwarding attempt, set up by open_data_connection(), *might* fail. Remove AI_ADDRCONFIG anyway. While at it, spell out AF_UNSPEC as well (in practice, this makes no difference, as Linux defines AF_UNSPEC as PF_UNSPEC as 0). While running the local test suite ("make -j10 check") on a host without a publicly routable IPv6 address, the "netstat -anp" output changes, due to this patch. Before:> Active Internet connections (servers and established) > Proto Recv-Q Send-Q Local Address Foreign Address State PID/Program name > tcp 0 0 127.0.0.1:58273 0.0.0.0:* LISTEN 51057/nbdkit > tcp 0 0 127.0.0.1:58272 0.0.0.0:* LISTEN 51050/nbdkit > tcp 0 0 127.0.0.1:58272 127.0.0.1:35332 ESTABLISHED 51050/nbdkit > tcp 0 0 127.0.0.1:35332 127.0.0.1:58272 ESTABLISHED 51204/nbdkitWe have two nbdkit processes (PIDs 51057 and 51050) listening over TCPv4, and a third one (PID 51204) connected to PID 51050 over TCPv4. After:> Active Internet connections (servers and established) > Proto Recv-Q Send-Q Local Address Foreign Address State PID/Program name > tcp 0 0 127.0.0.1:54146 0.0.0.0:* LISTEN 49310/nbdkit > tcp 0 0 127.0.0.1:54145 0.0.0.0:* LISTEN 49303/nbdkit > tcp6 0 0 ::1:54145 :::* LISTEN 49303/nbdkit > tcp6 0 0 ::1:54146 :::* LISTEN 49310/nbdkit > tcp6 0 0 ::1:57432 ::1:54145 ESTABLISHED 49457/nbdkit > tcp6 0 0 ::1:54145 ::1:57432 ESTABLISHED 49303/nbdkitThe two listening nbdkit processes (PIDs 49310 and 49303) are now doing so over both TCPv4 and TCPv6, and the third one (PID 49457) actually connects to PID 49303 over TCPv6! 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> --- nbd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nbd.c b/nbd.c index dcedd0a52dce..92fad34c0e32 100644 --- a/nbd.c +++ b/nbd.c @@ -262,90 +262,91 @@ static int bind_tcpip_socket (const char *port, int **fds_rtn, size_t *nr_fds_rtn) { struct addrinfo *ai = NULL; struct addrinfo hints; struct addrinfo *a; int err; int *fds = NULL; size_t nr_fds; int addr_in_use = 0; memset (&hints, 0, sizeof hints); - hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; + hints.ai_family = AF_UNSPEC; + hints.ai_flags = AI_PASSIVE; hints.ai_socktype = SOCK_STREAM; err = getaddrinfo ("localhost", port, &hints, &ai); if (err != 0) { #if DEBUG_STDERR fprintf (stderr, "%s: getaddrinfo: localhost: %s: %s", g_get_prgname (), port, gai_strerror (err)); #endif return -1; } nr_fds = 0; for (a = ai; a != NULL; a = a->ai_next) { int sock, opt; sock = socket (a->ai_family, a->ai_socktype, a->ai_protocol); if (sock == -1) error (EXIT_FAILURE, errno, "socket"); opt = 1; if (setsockopt (sock, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof opt) == -1) perror ("setsockopt: SO_REUSEADDR"); #ifdef IPV6_V6ONLY if (a->ai_family == PF_INET6) { if (setsockopt (sock, IPPROTO_IPV6, IPV6_V6ONLY, &opt, sizeof opt) == -1) perror ("setsockopt: IPv6 only"); } #endif if (bind (sock, a->ai_addr, a->ai_addrlen) == -1) { if (errno == EADDRINUSE) { addr_in_use = 1; close (sock); continue; } perror ("bind"); close (sock); continue; } if (listen (sock, SOMAXCONN) == -1) { perror ("listen"); close (sock); continue; } nr_fds++; fds = realloc (fds, sizeof (int) * nr_fds); if (!fds) error (EXIT_FAILURE, errno, "realloc"); fds[nr_fds-1] = sock; } freeaddrinfo (ai); if (nr_fds == 0 && addr_in_use) { #if DEBUG_STDERR fprintf (stderr, "%s: unable to bind to localhost:%s: %s\n", g_get_prgname (), port, strerror (EADDRINUSE)); #endif return -1; } #if DEBUG_STDERR fprintf (stderr, "%s: bound to localhost:%s (%zu socket(s))\n", g_get_prgname (), port, nr_fds); #endif *fds_rtn = fds; *nr_fds_rtn = nr_fds; return 0; } /** * Wait for nbdkit to start and be listening for connections. */ -- 2.19.1.3.g30247aa5d201
Richard W.M. Jones
2022-Mar-31 12:13 UTC
[Libguestfs] [p2v PATCH 00/10] Simplify NBD server management
One thing I realised over the weekend is that if we're removing qemu-nbd support, then we'd better have an nbdkit available on older RHEL. I pushed a few small fixes to nbdkit this morning, and I'm happy to report that current nbdkit from git compiles fine on RHEL 6, albeit with a reduced set of features. I didn't try RHEL 5, but hardware which is only supported by RHEL 5 and not RHEL 6 would be 12-15 years old at this point. Nevertheless we can probably make nbdkit work on RHEL 5 if we have to. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org