Currently we #define NETWORK_ADDRESS as "169.254.0.0". That's a bug in libguestfs, but it's been invisible -- thus far it's been canceled out by *independent* bugs in *both* QEMU and libvirt. (1) With the direct backend, the current definition of NETWORK_ADDRESS results in the following QEMU command line option: -netdev user,id=usernet,net=169.254.0.0/16 According to the QEMU documentation, the "net" property here is supposed to specify the *exact* IP address that the guest will receive. The guest however receives 169.254.2.15 (I've checked that with virt-rescue). In other words, libguestfs doesn't do what the QEMU documentation says, and QEMU doesn't do what the QEMU documentation says either. The end result has been good enough -- but only until now. (2) With the libvirt backend, the current definition of NETWORK_ADDRESS results in the following domain XML snippet: <interface type="user"> <model type="virtio"/> <ip family="ipv4" address="169.254.0.0" prefix="16"/> </interface> which libvirt translates, in turn, to -netdev {"type":"user","net":"169.254.0.0/16","id":"hostnet0"} According to the domain XML documentation, the @address attribute here is again supposed to specify the *exact* IP address that the guest will receive. However, the guest receives 169.254.2.15 (I've checked that with virt-rescue). In other words, libguestfs doesn't do what the libvirt documentation says, and libvirt doesn't do what the libvirt documentation says either. The end result has been good enough -- but only until now. Where things break down though is the subsequent passt enablement, in the rest of this series. For example, when using the libvirt backend together with passt, libvirt translates the @address attribute to passt's "--address" option, but passt takes the address *verbatim*, and not as a subnet base address. That difference is visible in the appliance; for example, when running virt-rescue on a Fedora 38 image, and issuing "ip addr". Namely, after enabling passt for the libvirt backend, the guest-visible IP address changes from 169.254.2.15 to 169.254.0.0, which is an IP address that makes no sense for an endpoint. Fix the latent bug by specifying the actual guest IP address we want, in NETWORK_ADDRESS. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> --- Notes: v2: - pick up Rich's R-b lib/guestfs-internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h index c7ef32277e93..4be351e3d3cc 100644 --- a/lib/guestfs-internal.h +++ b/lib/guestfs-internal.h @@ -155,7 +155,7 @@ cleanup_mutex_unlock (pthread_mutex_t **ptr) /* Network address and network mask (expressed as address prefix) that the * appliance will see (if networking is enabled). */ -#define NETWORK_ADDRESS "169.254.0.0" +#define NETWORK_ADDRESS "169.254.2.15" #define NETWORK_PREFIX "16" /* Guestfs handle and associated structures. */

Commit 55202a4d49a1 ("New API: get-sockdir", 2016-02-03) added identical language to "fish/guestfish.pod" and "src/guestfs.pod", including an internal link L</get-sockdir>. That's appropriate for "fish/guestfish.pod", but the same API description is generated with a different anchor for "src/guestfs.pod". Adapt the reference. Fixes: 55202a4d49a101392148d79cb2e1591428db2681 Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> --- Notes: v2: - pick up Rich's R-b lib/guestfs.pod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/guestfs.pod b/lib/guestfs.pod index c6c8cb16860c..68688f31aa5f 100644 --- a/lib/guestfs.pod +++ b/lib/guestfs.pod @@ -3223,7 +3223,7 @@ non-essential runtime files. If it is set, then is used to store temporary sockets. Otherwise, F</tmp> is used. -See also L</get-sockdir>, +See also L</guestfs_get_sockdir>, L<http://www.freedesktop.org/wiki/Specifications/basedir-spec/>. =back

Consider the following inverted call tree (effectively a dependency tree -- callees are at the top and near the left margin): lazy_make_tmpdir() [lib/tmpdirs.c] guestfs_int_lazy_make_tmpdir() [lib/tmpdirs.c] guestfs_int_make_temp_path() [lib/tmpdirs.c] guestfs_int_lazy_make_sockdir() [lib/tmpdirs.c] guestfs_int_create_socketname() [lib/launch.c] lazy_make_tmpdir() is our common workhorse / helper function that centralizes the mkdtemp() function call. guestfs_int_lazy_make_tmpdir() and guestfs_int_lazy_make_sockdir() are the next level functions, both calling lazy_make_tmpdir(), just feeding it different dirname generator functions, and different "is_runtime_dir" qualifications. These functions create temp dirs for various, more specific, purposes (see the manual and "lib/guestfs-internal.h" for more details). On a yet higher level are guestfs_int_make_temp_path() and guestfs_int_create_socketname() -- they serve for creating *entries* in those specific temp directories. The discrepancy here is that, although all the other functions live in "lib/tmpdirs.c", guestfs_int_create_socketname() is defined in "lib/launch.c". That makes for a confusing code reading; move the function to "lib/tmpdirs.c", just below its sibling function guestfs_int_make_temp_path(). While at it, correct the leading comment on guestfs_int_create_socketname() -- the socket pathname is created in the socket directory, not in the temporary directory. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> --- Notes: v2: - pick up Rich's R-b lib/guestfs-internal.h | 2 +- lib/launch.c | 26 -------------------- lib/tmpdirs.c | 26 ++++++++++++++++++++ 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h index a6c4266ad3fe..2ee16ea1e75a 100644 --- a/lib/guestfs-internal.h +++ b/lib/guestfs-internal.h @@ -668,6 +668,7 @@ extern int guestfs_int_set_env_runtimedir (guestfs_h *g, const char *envname, co extern int guestfs_int_lazy_make_tmpdir (guestfs_h *g); extern int guestfs_int_lazy_make_sockdir (guestfs_h *g); extern char *guestfs_int_make_temp_path (guestfs_h *g, const char *name, const char *extension); +extern int guestfs_int_create_socketname (guestfs_h *g, const char *filename, char (*sockname)[UNIX_PATH_MAX]); extern char *guestfs_int_lazy_make_supermin_appliance_dir (guestfs_h *g); extern void guestfs_int_remove_tmpdir (guestfs_h *g); extern void guestfs_int_remove_sockdir (guestfs_h *g); @@ -700,7 +701,6 @@ extern int guestfs_int_get_uefi (guestfs_h *g, char *const *firmwares, const cha extern int64_t guestfs_int_timeval_diff (const struct timeval *x, const struct timeval *y); extern void guestfs_int_launch_send_progress (guestfs_h *g, int perdozen); extern void guestfs_int_unblock_sigterm (void); -extern int guestfs_int_create_socketname (guestfs_h *g, const char *filename, char (*sockname)[UNIX_PATH_MAX]); extern void guestfs_int_register_backend (const char *name, const struct backend_ops *); extern int guestfs_int_set_backend (guestfs_h *g, const char *method); extern bool guestfs_int_passt_runnable (guestfs_h *g); diff --git a/lib/launch.c b/lib/launch.c index 87fb10e75a6c..b9b76e509162 100644 --- a/lib/launch.c +++ b/lib/launch.c @@ -310,32 +310,6 @@ guestfs_impl_config (guestfs_h *g, return 0; } -/** - * Create the path for a socket with the selected filename in the - * tmpdir. - */ -int -guestfs_int_create_socketname (guestfs_h *g, const char *filename, - char (*sockpath)[UNIX_PATH_MAX]) -{ - int r; - - if (guestfs_int_lazy_make_sockdir (g) == -1) - return -1; - - r = snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->sockdir, filename); - if (r >= UNIX_PATH_MAX) { - error (g, _("socket path too long: %s/%s"), g->sockdir, filename); - return -1; - } - if (r < 0) { - perrorf (g, _("%s"), g->sockdir); - return -1; - } - - return 0; -} - /** * When the library is loaded, each backend calls this function to * register itself in a global list. diff --git a/lib/tmpdirs.c b/lib/tmpdirs.c index b8e19de2bf9e..24adf98daee0 100644 --- a/lib/tmpdirs.c +++ b/lib/tmpdirs.c @@ -253,6 +253,32 @@ guestfs_int_make_temp_path (guestfs_h *g, extension ? extension : ""); } +/** + * Create the path for a socket with the selected filename in the + * sockdir. + */ +int +guestfs_int_create_socketname (guestfs_h *g, const char *filename, + char (*sockpath)[UNIX_PATH_MAX]) +{ + int r; + + if (guestfs_int_lazy_make_sockdir (g) == -1) + return -1; + + r = snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->sockdir, filename); + if (r >= UNIX_PATH_MAX) { + error (g, _("socket path too long: %s/%s"), g->sockdir, filename); + return -1; + } + if (r < 0) { + perrorf (g, _("%s"), g->sockdir); + return -1; + } + + return 0; +} + /** * Create the supermin appliance directory under cachedir, if it does * not exist.

On QEMU 7.2.0+, if "passt" is available, ask QEMU for passt ("stream") rather than SLIRP ("user") networking. For this, we need to run passt ourselves. Given that passt daemonizes by default, start it with our traditional function guestfs_int_cmd_run(). Ask passt to save its PID file, because in case something goes wrong before we're completely sure the appliance (i.e. QEMU) is up and running, we'll need to kill passt, the *grandchild*, ourselves. Pass "--one-off" to passt (same as libvirt). This way, once we have proof that QEMU has connected to passt (because the appliance shows signs of life), we need not clean up passt ourselves -- once QEMU exits, passt will see an EOF on the unix domain socket, and exit as well. Passt is way more flexible than SLIRP, and passt normally intends to imitate the host environment in the guest as much as possible. This means that, when switching from SLIRP to passt, the guest would see changes to the following: - guest IP address, - guest subnet mask, - host (= gateway) IP address, - host (= gateway) MAC address. Extract the SLIRP defaults into the new macros NETWORK_GW_IP and NETWORK_GW_MAC, and pass them explicitly to passt. In particular, "tests/rsync/test-rsync.sh" fails without setting the host address (NETWORK_GW_IP) properly. (These artifacts can be verified in the appliance with "virt-rescue --network", by running "ip addr", "ip route", and "ip neighbor" at the virt-rescue prompt. There are four scenarios: two libguest backends, times passt being installed or not installed.) Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - document the constants returned by launch_passt() with C<> notation, in the function's leading comment - don't assert WIFEXITED just because of !WIFSIGNALED; check explicitly [Rich] lib/guestfs-internal.h | 26 ++++ lib/launch-direct.c | 152 +++++++++++++++++++- 2 files changed, 173 insertions(+), 5 deletions(-) diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h index 9ba4d4ad46cf..9f4f800e6d6e 100644 --- a/lib/guestfs-internal.h +++ b/lib/guestfs-internal.h @@ -158,6 +158,32 @@ cleanup_mutex_unlock (pthread_mutex_t **ptr) #define NETWORK_ADDRESS "169.254.2.15" #define NETWORK_PREFIX "16" +/* The IP address and the MAC address of the host, as seen from the appliance. + * + * NETWORK_GW_IP should be the same as NETWORK_ADDRESS, only replacing ".15" in + * the rightmost octet with ".2". NETWORK_GW_MAC cannot be changed. These + * restrictions are a consequence of the following landscape: + * + * libguestfs backend userspace network stack restrictions + * ------------------ ----------------------- -------------------------------- + * direct passt None; both NETWORK_GW_IP and + * NETWORK_GW_MAC can be set on the + * passt command line. + * + * direct SLIRP SLIRP hard-codes NETWORK_GW_MAC. + * + * libvirt passt The domain XML does not expose + * either knob (RHBZ#2222766), even + * though passt could accept both. + * + * libvirt SLIRP The domain XML does not expose + * either knob (RHBZ#2222766), and + * SLIRP hard-codes NETWORK_GW_MAC + * anyway. + */ +#define NETWORK_GW_IP "169.254.2.2" +#define NETWORK_GW_MAC "52:56:00:00:00:02" + /* Guestfs handle and associated structures. */ /* State. */ diff --git a/lib/launch-direct.c b/lib/launch-direct.c index 3f46f0509736..cdfd25a9afed 100644 --- a/lib/launch-direct.c +++ b/lib/launch-direct.c @@ -32,6 +32,7 @@ #include <errno.h> #include <fcntl.h> #include <sys/types.h> +#include <sys/wait.h> #include <sys/time.h> #include <sys/resource.h> #include <sys/stat.h> @@ -48,6 +49,7 @@ #include "guestfs-internal.h" #include "guestfs_protocol.h" #include "qemuopts.h" +#include "ignore-value.h" /* Per-handle data. */ struct backend_direct_data { @@ -333,6 +335,115 @@ add_drives (guestfs_h *g, struct backend_direct_data *data, return 0; } +/** + * Launch passt such that it daemonizes. + * + * On error, C<-1> is returned; C<passt_pid> and C<sockpath> are not modified. + * + * On success, C<0> is returned. C<passt_pid> contains the PID of the passt + * background process. C<sockpath> contains the pathname of the unix domain + * socket where passt will accept a single connection. + */ +static int +launch_passt (guestfs_h *g, long *passt_pid, char (*sockpath)[UNIX_PATH_MAX]) +{ + int rc; + char sockpath_local[sizeof *sockpath]; + char *pid_path; + struct command *cmd; + int passt_status; + int passt_exit; + char *pid_str; + long passt_pid_local; + char *endptr; + + rc = -1; + if (guestfs_int_create_socketname (g, "passt.sock", &sockpath_local) == -1) + return rc; + + pid_path = guestfs_int_make_pid_path (g, "passt"); + if (pid_path == NULL) + return rc; + + cmd = guestfs_int_new_command (g); + if (cmd == NULL) + goto free_pid_path; + + guestfs_int_cmd_add_arg (cmd, "passt"); + guestfs_int_cmd_add_arg (cmd, "--one-off"); + guestfs_int_cmd_add_arg (cmd, "--socket"); + guestfs_int_cmd_add_arg (cmd, sockpath_local); + guestfs_int_cmd_add_arg (cmd, "--pid"); + guestfs_int_cmd_add_arg (cmd, pid_path); + guestfs_int_cmd_add_arg (cmd, "--address"); + guestfs_int_cmd_add_arg (cmd, NETWORK_ADDRESS); + guestfs_int_cmd_add_arg (cmd, "--netmask"); + guestfs_int_cmd_add_arg (cmd, NETWORK_PREFIX); + guestfs_int_cmd_add_arg (cmd, "--mac-addr"); + guestfs_int_cmd_add_arg (cmd, NETWORK_GW_MAC); + guestfs_int_cmd_add_arg (cmd, "--gateway"); + guestfs_int_cmd_add_arg (cmd, NETWORK_GW_IP); + + passt_status = guestfs_int_cmd_run (cmd); + if (passt_status == -1) + /* guestfs_int_cmd_run() reports errors internally, so just bail here */ + goto close_cmd; + + if (WIFSIGNALED (passt_status)) { + error (g, _("passt was killed with signal %d"), WTERMSIG (passt_status)); + goto close_cmd; + } + + if (!WIFEXITED (passt_status)) { + error (g, _("internal error: unexpected exit status from passt (%d)"), + passt_status); + goto close_cmd; + } + + passt_exit = WEXITSTATUS (passt_status); + if (passt_exit != 0) { + error (g, _("passt exited with status %d"), passt_exit); + goto close_cmd; + } + + /* At this point passt has forked into the background, dropped privileges, and + * written a PID file. Due to "--one-off", passt will exit once our QEMU + * appliance disappears (forcibly or cleanly); however, we still need the + * passt PID *temporarily*, so we can kill passt in case we encounter an error + * *before* starting the appliance. + */ + if (guestfs_int_read_whole_file (g, pid_path, &pid_str, NULL) == -1) + /* Any error has been reported internally, so just bail. We can't kill + * passt here because we've failed to get its PID in the first place... + */ + goto close_cmd; + + errno = 0; + passt_pid_local = strtol (pid_str, &endptr, 10); + if (endptr == pid_str || (*endptr != '\0' && *endptr != '\n') || errno != 0 || + passt_pid_local <= 1) { + /* Same thing, we can't kill passt just yet. */ + error (g, _("failed to parse passt PID from '%s'"), pid_path); + goto free_pid_str; + } + + /* We're done. */ + *passt_pid = passt_pid_local; + ignore_value (strcpy (*sockpath, sockpath_local)); + rc = 0; + +free_pid_str: + free (pid_str); + +close_cmd: + guestfs_int_cmd_close (cmd); + +free_pid_path: + free (pid_path); + + return rc; +} + static int launch_direct (guestfs_h *g, void *datav, const char *arg) { @@ -340,6 +451,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) struct qemuopts *qopts = NULL; int daemon_accept_sock = -1, console_sock = -1; int r; + long passt_pid = -1; int flags; int sv[2]; struct sockaddr_un addr; @@ -658,11 +770,30 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) /* Enable user networking. */ if (g->enable_network) { - start_list ("-netdev") { - append_list ("user"); - append_list ("id=usernet"); - append_list ("net=" NETWORK_ADDRESS "/" NETWORK_PREFIX); - } end_list (); + /* If qemu is 7.2.0+ and "passt" is available, ask for passt rather + * than SLIRP. RHBZ#2184967. + */ + if (guestfs_int_version_ge (&data->qemu_version, 7, 2, 0) && + guestfs_int_passt_runnable (g)) { + char passt_sock[UNIX_PATH_MAX]; + + if (launch_passt (g, &passt_pid, &passt_sock) == -1) + goto cleanup0; + + start_list ("-netdev") { + append_list ("stream"); + append_list ("id=usernet"); + append_list ("addr.type=unix"); + append_list_format ("addr.path=%s", passt_sock); + } end_list (); + } + else { + start_list ("-netdev") { + append_list ("user"); + append_list ("id=usernet"); + append_list ("net=" NETWORK_ADDRESS "/" NETWORK_PREFIX); + } end_list (); + } start_list ("-device") { append_list (VIRTIO_DEVICE_NAME ("virtio-net")); append_list ("netdev=usernet"); @@ -893,6 +1024,15 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) debug (g, "appliance is up"); + /* From this point onward, even if we fail, QEMU terminating (forcefully or + * gracefully) will cause passt to go away as well. Note that we can't + * precisely tell whether QEMU managed to open the passt socket before QEMU + * failed. Therefore, err on the side of killing passt needlessly, rather + * than not killing it when needed -- that's why we re-set "passt_pid" to (-1) + * only this late during QEMU startup verification. + */ + passt_pid = -1; + /* This is possible in some really strange situations, such as * guestfsd starts up OK but then qemu immediately exits. Check for * it because the caller is probably expecting to be able to send @@ -924,6 +1064,8 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) data->qemu_data = NULL; cleanup0: + if (passt_pid != -1) + kill (passt_pid, SIGTERM); if (qopts != NULL) qemuopts_free (qopts); if (daemon_accept_sock >= 0)