Libguestfs - Jul 2023 - [libguestfs PATCH 5/7] lib: move guestfs_int_create_socketname() from "launch.c" to "tmpdirs.c"

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967

This series makes both backends prefer passt over slirp for appliance
networking, if QEMU or libvirt (respectively) is recent enough, and
passt is installed.

My test setup is:

$ virt-builder fedora-38

Then, each test run looks like this:

Terminal#1:

$ ./run rescue/virt-rescue --network --ro -a fedora-38.img

Terminal#2:

- check if "passt" is running (ps -ef, pgrep, ...), provided it
*should*
  be running

Terminal#1:
><rescue> ping -c 3 8.8.8.8
><rescue> ip neighbor
><rescue> ip addr
><rescue> ip route
Expected results (in the above order), always on the "eth0" lines:
- all three pings succeed (get replies)
- 52:56:00:00:00:02
- 169.254.2.15/16
- default via 169.254.2.2

Actual results:

(1) At current master (13c7052ff96d):

(1.1) With "LIBGUESTFS_BACKEND=direct":

      Pass, this is the baseline.

(1.2) With "LIBGUESTFS_BACKEND=libvirt":

      Pass, this is the baseline.

(2) With this series applied:

(2.1) With passt not installed:

(2.1.1) With "LIBGUESTFS_BACKEND=direct":

        Pass, this is a regression test concerning the absence of
        "passt".

(2.1.2) With "LIBGUESTFS_BACKEND=libvirt":

        Pass, this is a regression test concerning the absence of
        "passt".

(2.2) With passt installed:

(2.2.1) With "LIBGUESTFS_BACKEND=direct":

        Pass, this verifies the new feature.

(2.2.2) With "LIBGUESTFS_BACKEND=libvirt":

        Basic connectivity works fine (i.e., the pings).

        The "ip neighbor" and "ip route" checks fail. In
addition, the
        "ip addr" check *partially* fails. All that is due to libvirt
        bugs:

        (a) Libvirt specifies the *guest MAC* (virtio-net MAC) as the
            *host MAC* for passt, with a wrong "--mac-addr" option
(from
            libvirt commit a56f0168d576, "qemu: hook up passt config to
            qemu domains", 2023-01-10). This breaks "ip
neighbor".

        (b) Libvirt doesn't pass the "--gateway" option to passt.
This
            breaks "ip route". Namely, passt (following its default
            behavior) sets the guest's gateway to 192.168.0.1, which is
            the gateway for my *host*.

        (c) "ip addr" also reports "169.254.2.15/1". The IP
address is
            fine, but the netmask is wrong; it's not /16. This is most
            likely a consequence of (b) -- normally the gateway is
            supposed to be on the same Ethernet segment (subnet) as the
            endpoint! 192 decimal is 11000000 binary, while 169 decimal
            is 10101001 binary, and their longest common initial bit
            sequence is just 1 bit -- hence the /1, most likely.

        I've filed
<https://bugzilla.redhat.com/show_bug.cgi?id=2222766>
        about these.

The upshot is that "appliance networking with passt" works with either
backend, but with the libvirt backend, there are some appliance-visible
differences from the SLIRP environment. Whether that's a problem in
practice, I can't tell (probably not a problem) -- dependent on future
decisions about RHBZ#2222766, we might want to update the libvirt
backend code introduced here.

Laszlo

Laszlo Ersek (7):
  lib: fix NETWORK_ADDRESS: make it an actual IP address, not a subnet
    base
  lib/launch-libvirt: support networking with passt
  docs: fix broken link in the guestfs manual
  docs: clarify sockdir's separation
  lib: move guestfs_int_create_socketname() from "launch.c" to
    "tmpdirs.c"
  lib: introduce guestfs_int_make_pid_path()
  lib/launch-direct: support networking with passt

 fish/guestfish.pod              |   4 +-
 generator/actions_properties.ml |   8 +-
 lib/guestfs-internal.h          |  32 ++++-
 lib/guestfs.pod                 |   6 +-
 lib/launch-direct.c             | 147 +++++++++++++++++++-
 lib/launch-libvirt.c            |  11 ++
 lib/launch.c                    |  54 +++----
 lib/tmpdirs.c                   |  41 ++++++
 8 files changed, 263 insertions(+), 40 deletions(-)


base-commit: 13c7052ff96d5ee99ec1b1252f1a3b4d7aed44d2

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 at redhat.com>
---
 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. */

We generate the <interface type="user"> element on libvirt
3.8.0+ already.

For selecting passt rather than SLIRP, we only need to insert the child
element <backend type='passt'>. Make that child element
conditional on
libvirt 9.0.0+, plus "passt --help" being executable.

For the latter, place the new helper function guestfs_int_passt_runnable()
in "lib/launch.c" -- we're going to use the same function for the
direct
backend as well.

This change exposes a number of (perceived) shortcomings in libvirt; I've
filed <https://bugzilla.redhat.com/show_bug.cgi?id=2222766> about those.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 lib/guestfs-internal.h |  1 +
 lib/launch-libvirt.c   | 11 ++++++++
 lib/launch.c           | 28 ++++++++++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 4be351e3d3cc..a6c4266ad3fe 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -703,6 +703,7 @@ 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);
 
 /* Close all file descriptors matching the condition. */
 #define close_file_descriptors(cond) do {                               \
diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c
index d4bf1a8ff242..994909a35f2d 100644
--- a/lib/launch-libvirt.c
+++ b/lib/launch-libvirt.c
@@ -1414,6 +1414,17 @@ construct_libvirt_xml_devices (guestfs_h *g,
         guestfs_int_version_ge (&params->data->libvirt_version, 3, 8,
0)) {
       start_element ("interface") {
         attribute ("type", "user");
+        /* If libvirt is 9.0.0+ and "passt" is available, ask for
passt rather
+         * than SLIRP (RHBZ#2184967).  Note that this causes some
+         * appliance-visible changes (although network connectivity is
certainly
+         * functional); refer to RHBZ#2222766 about those.
+         */
+        if (guestfs_int_version_ge (&params->data->libvirt_version,
9, 0, 0) &&
+            guestfs_int_passt_runnable (g)) {
+          start_element ("backend") {
+            attribute ("type", "passt");
+          } end_element ();
+        }
         start_element ("model") {
           attribute ("type", "virtio");
         } end_element ();
diff --git a/lib/launch.c b/lib/launch.c
index 6e08b12006da..94c8f676d8bd 100644
--- a/lib/launch.c
+++ b/lib/launch.c
@@ -36,6 +36,7 @@
 #include <signal.h>
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 #include <errno.h>
 #include <assert.h>
 #include <libintl.h>
@@ -414,6 +415,33 @@ guestfs_int_set_backend (guestfs_h *g, const char *method)
   return 0;
 }
 
+/**
+ * Return C<true> if we can call S<C<passt --help>>, and it
exits with status
+ * C<0> or C<1>.
+ *
+ * (At least C<passt-0^20230222.g4ddbcb9-2.el9_2.x86_64> terminates with
status
+ * C<1> in response to "--help", which is arguably wrong, and
potentially
+ * subject to change, but it doesn't really matter.)
+ */
+bool
+guestfs_int_passt_runnable (guestfs_h *g)
+{
+  CLEANUP_CMD_CLOSE struct command *cmd = NULL;
+  int r, ex;
+
+  cmd = guestfs_int_new_command (g);
+  if (cmd == NULL)
+    return false;
+
+  guestfs_int_cmd_add_string_unquoted (cmd, "passt --help");
+  r = guestfs_int_cmd_run (cmd);
+  if (r == -1 || !WIFEXITED (r))
+    return false;
+
+  ex = WEXITSTATUS (r);
+  return ex == 0 || ex == 1;
+}
+
 /* This hack is only required to make static linking work.  See:
  *
https://stackoverflow.com/questions/1202494/why-doesnt-attribute-constructor-work-in-a-static-library
  */

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 at redhat.com>
---
 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

There's another reason for separating sockdir from tmpdir, beyond
"shorter
pathnames needed": permissions. For example, passt drops privileges such
that it cannot access "/tmp", and that restricts both the unix domain
socket and the PID file of passt.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 generator/actions_properties.ml | 8 ++++++--
 fish/guestfish.pod              | 4 ++--
 lib/guestfs.pod                 | 4 ++--
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/generator/actions_properties.ml b/generator/actions_properties.ml
index f84afb10d674..42eaaa4d81e1 100644
--- a/generator/actions_properties.ml
+++ b/generator/actions_properties.ml
@@ -595,13 +595,17 @@ Get the handle identifier.  See
C<guestfs_set_identifier>." };
     name = "get_sockdir"; added = (1, 33, 8);
     style = RString (RPlainString, "sockdir"), [], [];
     blocking = false;
-    shortdesc = "get the temporary directory for sockets";
+    shortdesc = "get the temporary directory for sockets and PID
files";
     longdesc = "\
-Get the directory used by the handle to store temporary socket files.
+Get the directory used by the handle to store temporary socket and PID
+files.
 
 This is different from C<guestfs_get_tmpdir>, as we need shorter
 paths for sockets (due to the limited buffers of filenames for UNIX
 sockets), and C<guestfs_get_tmpdir> may be too long for them.
+Furthermore, sockets and PID files must be accessible to such background
+services started by libguestfs that may not have permission to access
+the temporary directory returned by C<guestfs_get_tmpdir>.
 
 The environment variable C<XDG_RUNTIME_DIR> controls the default
 value: If C<XDG_RUNTIME_DIR> is set, then that is the default.
diff --git a/fish/guestfish.pod b/fish/guestfish.pod
index ccc0825b84a0..492aa7163fcb 100644
--- a/fish/guestfish.pod
+++ b/fish/guestfish.pod
@@ -1548,8 +1548,8 @@ See L</LIBGUESTFS_CACHEDIR>,
L</LIBGUESTFS_TMPDIR>.
 This directory represents a user-specific directory for storing
 non-essential runtime files.
 
-If it is set, then is used to store temporary sockets.  Otherwise,
-F</tmp> is used.
+If it is set, then is used to store temporary sockets and PID files.
+Otherwise, F</tmp> is used.
 
 See also L</get-sockdir>,
 L<http://www.freedesktop.org/wiki/Specifications/basedir-spec/>.
diff --git a/lib/guestfs.pod b/lib/guestfs.pod
index 68688f31aa5f..e46dd81f9e0a 100644
--- a/lib/guestfs.pod
+++ b/lib/guestfs.pod
@@ -3220,8 +3220,8 @@ See L</LIBGUESTFS_CACHEDIR>,
L</LIBGUESTFS_TMPDIR>.
 This directory represents a user-specific directory for storing
 non-essential runtime files.
 
-If it is set, then is used to store temporary sockets.  Otherwise,
-F</tmp> is used.
+If it is set, then is used to store temporary sockets and PID files.
+Otherwise, F</tmp> is used.
 
 See also L</guestfs_get_sockdir>,
 L<http://www.freedesktop.org/wiki/Specifications/basedir-spec/>.

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 at redhat.com>
---
 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 94c8f676d8bd..a0a8e1c45a51 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.

Introduce a small function for creating pathnames for PID files.

guestfs_int_make_pid_path() is something of an amalgamation of
guestfs_int_make_temp_path() [1] and guestfs_int_create_socketname() [2]:

- it creates a pathname under sockdir, like [2],

- it uses the handle's unique counter, like [1],

- it takes a name like both [1] and [2], but the name is not size-limited
  like in [2], plus we hardcode the suffix from [1] as ".pid".

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 lib/guestfs-internal.h |  1 +
 lib/tmpdirs.c          | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 2ee16ea1e75a..9ba4d4ad46cf 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -669,6 +669,7 @@ 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_make_pid_path (guestfs_h *g, const char *name);
 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);
diff --git a/lib/tmpdirs.c b/lib/tmpdirs.c
index 24adf98daee0..22b8f54b0693 100644
--- a/lib/tmpdirs.c
+++ b/lib/tmpdirs.c
@@ -279,6 +279,21 @@ guestfs_int_create_socketname (guestfs_h *g, const char
*filename,
   return 0;
 }
 
+/**
+ * Generate unique paths for PID files.
+ *
+ * Returns a unique path or NULL on error.  On success, the pathname points
+ * under sockdir and not tmpdir; daemons that write PID files after dropping
+ * privileges may not have access to tmpdir.
+ */
+char *
+guestfs_int_make_pid_path (guestfs_h *g, const char *name)
+{
+  if (guestfs_int_lazy_make_sockdir (g) < 0)
+    return NULL;
+  return safe_asprintf (g, "%s/%s%d.pid", g->sockdir, name,
++g->unique);
+}
+
 /**
  * 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 at redhat.com>
---
 lib/guestfs-internal.h |  26 ++++
 lib/launch-direct.c    | 147 +++++++++++++++++++-
 2 files changed, 168 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..8d6ad025a4e1 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,110 @@ add_drives (guestfs_h *g, struct backend_direct_data
*data,
   return 0;
 }
 
+/**
+ * Launch passt such that it daemonizes.
+ *
+ * On error, -1 is returned; C<passt_pid> and C<sockpath> are not
modified.
+ *
+ * On success, 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;
+  }
+
+  assert (WIFEXITED (passt_status));
+  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 +446,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 +765,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 +1019,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 +1059,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)

On Thu, Jul 13, 2023 at 07:10:45PM +0200, Laszlo Ersek
wrote:
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2184967
> 
> This series makes both backends prefer passt over slirp for appliance
> networking, if QEMU or libvirt (respectively) is recent enough, and
> passt is installed.
> 
> My test setup is:
> 
> $ virt-builder fedora-38
> 
> Then, each test run looks like this:
> 
> Terminal#1:
> 
> $ ./run rescue/virt-rescue --network --ro -a fedora-38.img
> 
> Terminal#2:
> 
> - check if "passt" is running (ps -ef, pgrep, ...), provided it
*should*
>   be running
> 
> Terminal#1:
> 
> ><rescue> ping -c 3 8.8.8.8
> ><rescue> ip neighbor
> ><rescue> ip addr
> ><rescue> ip route
> 
> Expected results (in the above order), always on the "eth0"
lines:
> - all three pings succeed (get replies)
> - 52:56:00:00:00:02
> - 169.254.2.15/16
> - default via 169.254.2.2
Another good test (and the one that really matters) would be something
like this on Terminal #1 ...
><rescue> chroot /sysroot bash
><rescue> dnf install emacs
(or some other package).  If dnf can see the Fedora repositories
despite the network setup being slightly wonky, that will mean that
virt-customize should work.
> Actual results:
> 
> (1) At current master (13c7052ff96d):
> 
> (1.1) With "LIBGUESTFS_BACKEND=direct":
> 
>       Pass, this is the baseline.
> 
> (1.2) With "LIBGUESTFS_BACKEND=libvirt":
> 
>       Pass, this is the baseline.
> 
> (2) With this series applied:
> 
> (2.1) With passt not installed:
> 
> (2.1.1) With "LIBGUESTFS_BACKEND=direct":
> 
>         Pass, this is a regression test concerning the absence of
>         "passt".
> 
> (2.1.2) With "LIBGUESTFS_BACKEND=libvirt":
> 
>         Pass, this is a regression test concerning the absence of
>         "passt".
> 
> (2.2) With passt installed:
> 
> (2.2.1) With "LIBGUESTFS_BACKEND=direct":
> 
>         Pass, this verifies the new feature.
> 
> (2.2.2) With "LIBGUESTFS_BACKEND=libvirt":
> 
>         Basic connectivity works fine (i.e., the pings).
> 
>         The "ip neighbor" and "ip route" checks fail.
In addition, the
>         "ip addr" check *partially* fails. All that is due to
libvirt
>         bugs:
> 
>         (a) Libvirt specifies the *guest MAC* (virtio-net MAC) as the
>             *host MAC* for passt, with a wrong "--mac-addr"
option (from
>             libvirt commit a56f0168d576, "qemu: hook up passt config
to
>             qemu domains", 2023-01-10). This breaks "ip
neighbor".
> 
>         (b) Libvirt doesn't pass the "--gateway" option to
passt. This
>             breaks "ip route". Namely, passt (following its
default
>             behavior) sets the guest's gateway to 192.168.0.1, which is
>             the gateway for my *host*.
> 
>         (c) "ip addr" also reports "169.254.2.15/1".
The IP address is
>             fine, but the netmask is wrong; it's not /16. This is most
>             likely a consequence of (b) -- normally the gateway is
>             supposed to be on the same Ethernet segment (subnet) as the
>             endpoint! 192 decimal is 11000000 binary, while 169 decimal
>             is 10101001 binary, and their longest common initial bit
>             sequence is just 1 bit -- hence the /1, most likely.
> 
>         I've filed
<https://bugzilla.redhat.com/show_bug.cgi?id=2222766>
>         about these.
It's good that this work has found a bunch of libvirt bugs!

Rich.
> The upshot is that "appliance networking with passt" works with
either
> backend, but with the libvirt backend, there are some appliance-visible
> differences from the SLIRP environment. Whether that's a problem in
> practice, I can't tell (probably not a problem) -- dependent on future
> decisions about RHBZ#2222766, we might want to update the libvirt
> backend code introduced here.
> 
> Laszlo
> 
> Laszlo Ersek (7):
>   lib: fix NETWORK_ADDRESS: make it an actual IP address, not a subnet
>     base
>   lib/launch-libvirt: support networking with passt
>   docs: fix broken link in the guestfs manual
>   docs: clarify sockdir's separation
>   lib: move guestfs_int_create_socketname() from "launch.c" to
>     "tmpdirs.c"
>   lib: introduce guestfs_int_make_pid_path()
>   lib/launch-direct: support networking with passt
> 
>  fish/guestfish.pod              |   4 +-
>  generator/actions_properties.ml |   8 +-
>  lib/guestfs-internal.h          |  32 ++++-
>  lib/guestfs.pod                 |   6 +-
>  lib/launch-direct.c             | 147 +++++++++++++++++++-
>  lib/launch-libvirt.c            |  11 ++
>  lib/launch.c                    |  54 +++----
>  lib/tmpdirs.c                   |  41 ++++++
>  8 files changed, 263 insertions(+), 40 deletions(-)
> 
> 
> base-commit: 13c7052ff96d5ee99ec1b1252f1a3b4d7aed44d2
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW