Libguestfs - Jan 2019 - Re: [PATCH v2] Revert "launch: libvirt: Use qemu-bridge-helper to implement a full network (RHBZ#1148012)."

Let's actually compile and test the patch this time, rather than
trusting the RHEL 7.6 patch to apply directly to head ...

Rich.

We've been carrying this exact patch in RHEL 7 for several years.  It
reverts the change made in 2014 where we switched to using the virbr0
bridge for libguestfs networking instead of SLIRP.  We thought SLIRP
was going to become unsupported in qemu, but recently there have been
more encouraging signs since it looks like SLIRP will be spun off as a
separate project, running as a modular process and properly secured
and supported.

This reverts commit 224de20b9a8d5ea56f6337f19b4ca237bb88eca0.
---
 lib/guestfs-internal.h | 11 +++++
 lib/guestfs.pod        | 10 -----
 lib/launch-direct.c    | 11 -----
 lib/launch-libvirt.c   | 91 ++++++++++--------------------------------
 4 files changed, 32 insertions(+), 91 deletions(-)

diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 3274dcdf0..57647d11c 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -136,6 +136,17 @@
 #define MACHINE_TYPE "pseries"
 #endif
 
+/* Differences in qemu device names on ARMv7 (virtio-mmio), s/390x
+ * (CCW) vs normal hardware with PCI.
+ */
+#if defined(__arm__)
+#define VIRTIO_DEVICE_NAME(type) type "-device"
+#elif defined(__s390x__)
+#define VIRTIO_DEVICE_NAME(type) type "-ccw"
+#else
+#define VIRTIO_DEVICE_NAME(type) type "-pci"
+#endif
+
 /* Guestfs handle and associated structures. */
 
 /* State. */
diff --git a/lib/guestfs.pod b/lib/guestfs.pod
index 3a9805c95..e1998c7eb 100644
--- a/lib/guestfs.pod
+++ b/lib/guestfs.pod
@@ -1551,16 +1551,6 @@ On Fedora, install C<kernel-debuginfo> for the
C<vmlinux> file
 (containing symbols).  Make sure the symbols precisely match the
 kernel being used.
 
-=head3 network_bridge
-
-The libvirt backend supports:
-
- export LIBGUESTFS_BACKEND_SETTINGS=network_bridge=virbrX
-
-This allows you to override the bridge that is connected to when the
-network is enabled.  The default is C<virbr0>.  See also
-L</guestfs_set_network>.
-
 =head2 ATTACHING TO RUNNING DAEMONS
 
 I<Note (1):> This is B<highly experimental> and has a tendency to
eat
diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index 1e51d2a45..aa42f4ade 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -49,17 +49,6 @@
 #include "guestfs_protocol.h"
 #include "qemuopts.h"
 
-/* Differences in qemu device names on ARMv7 (virtio-mmio), s/390x
- * (CCW) vs normal hardware with PCI.
- */
-#if defined(__arm__)
-#define VIRTIO_DEVICE_NAME(type) type "-device"
-#elif defined(__s390x__)
-#define VIRTIO_DEVICE_NAME(type) type "-ccw"
-#else
-#define VIRTIO_DEVICE_NAME(type) type "-pci"
-#endif
-
 /* Per-handle data. */
 struct backend_direct_data {
   pid_t pid;                    /* Qemu PID. */
diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c
index 12dc2dbf6..839b39384 100644
--- a/lib/launch-libvirt.c
+++ b/lib/launch-libvirt.c
@@ -126,7 +126,6 @@ struct backend_libvirt_data {
   char *selinux_label;
   char *selinux_imagelabel;
   bool selinux_norelabel_disks;
-  char *network_bridge;
   char name[DOMAIN_NAME_LEN];   /* random name */
   bool is_kvm;                  /* false = qemu, true = kvm (from
capabilities)*/
   struct version libvirt_version; /* libvirt version */
@@ -167,7 +166,6 @@ static int is_blk (const char *path);
 static void ignore_errors (void *ignore, virErrorPtr ignore2);
 static void set_socket_create_context (guestfs_h *g);
 static void clear_socket_create_context (guestfs_h *g);
-static int check_bridge_exists (guestfs_h *g, const char *brname);
 
 #if HAVE_LIBSELINUX
 static void selinux_warning (guestfs_h *g, const char *func, const char
*selinux_op, const char *data);
@@ -448,17 +446,8 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
     guestfs_get_backend_setting (g, "internal_libvirt_imagelabel");
   data->selinux_norelabel_disks      guestfs_int_get_backend_setting_bool
(g, "internal_libvirt_norelabel_disks");
-  if (g->enable_network) {
-    data->network_bridge -      guestfs_get_backend_setting (g,
"network_bridge");
-    if (!data->network_bridge)
-      data->network_bridge = safe_strdup (g, "virbr0");
-  }
   guestfs_pop_error_handler (g);
 
-  if (g->enable_network && check_bridge_exists (g,
data->network_bridge) == -1)
-    goto cleanup;
-
   /* Locate and/or build the appliance. */
   TRACE0 (launch_build_libvirt_appliance_start);
 
@@ -1276,19 +1265,6 @@ construct_libvirt_xml_devices (guestfs_h *g,
       } end_element ();
     } end_element ();
 
-    /* Connect to libvirt bridge (see: RHBZ#1148012). */
-    if (g->enable_network) {
-      start_element ("interface") {
-        attribute ("type", "bridge");
-        start_element ("source") {
-          attribute ("bridge", params->data->network_bridge);
-        } end_element ();
-        start_element ("model") {
-          attribute ("type", "virtio");
-        } end_element ();
-      } end_element ();
-    }
-
     /* Libvirt adds some devices by default.  Indicate to libvirt
      * that we don't want them.
      */
@@ -1693,6 +1669,27 @@ construct_libvirt_xml_qemu_cmdline (guestfs_h *g,
       attribute ("value", tmpdir);
     } end_element ();
 
+    /* Workaround because libvirt user networking cannot specify
"net="
+     * parameter.
+     */
+    if (g->enable_network) {
+      start_element ("qemu:arg") {
+        attribute ("value", "-netdev");
+      } end_element ();
+
+      start_element ("qemu:arg") {
+        attribute ("value",
"user,id=usernet,net=169.254.0.0/16");
+      } end_element ();
+
+      start_element ("qemu:arg") {
+        attribute ("value", "-device");
+      } end_element ();
+
+      start_element ("qemu:arg") {
+        attribute ("value", VIRTIO_DEVICE_NAME
("virtio-net") ",netdev=usernet");
+      } end_element ();
+    }
+
     /* The qemu command line arguments requested by the caller. */
     for (hp = g->hv_params; hp; hp = hp->next) {
       start_element ("qemu:arg") {
@@ -1929,49 +1926,6 @@ is_blk (const char *path)
   return S_ISBLK (statbuf.st_mode);
 }
 
-static int
-is_dir (const char *path)
-{
-  struct stat statbuf;
-
-  if (stat (path, &statbuf) == -1)
-    return 0;
-  return S_ISDIR (statbuf.st_mode);
-}
-
-/* Used to check the network_bridge exists, or give a useful error
- * message.
- */
-static int
-check_bridge_exists (guestfs_h *g, const char *brname)
-{
-  CLEANUP_FREE char *path = NULL;
-
-  /* If this doesn't look like Linux, give up. */
-  if (!is_dir ("/sys/class/net"))
-    return 0;
-
-  /* Does the interface exist and is it a bridge? */
-  path = safe_asprintf (g, "/sys/class/net/%s/bridge", brname);
-  if (is_dir (path))
-    return 0;
-
-  error (g,
-         _("bridge ‘%s’ not found.  Try running:\n"
-           "\n"
-           "  brctl show\n"
-           "\n"
-           "to get a list of bridges on the host, and then selecting
the\n"
-           "bridge you wish the appliance network to connect to
using:\n"
-           "\n"
-           "  export LIBGUESTFS_BACKEND_SETTINGS=network_bridge=<bridge
name>\n"
-           "\n"
-           "You may also need to allow the bridge in
/etc/qemu/bridge.conf.\n"
-           "For further information see guestfs(3)."),
-         brname);
-  return -1;
-}
-
 static void
 ignore_errors (void *ignore, virErrorPtr ignore2)
 {
@@ -2017,9 +1971,6 @@ shutdown_libvirt (guestfs_h *g, void *datav, int
check_for_errors)
   free (data->selinux_imagelabel);
   data->selinux_imagelabel = NULL;
 
-  free (data->network_bridge);
-  data->network_bridge = NULL;
-
   for (i = 0; i < data->nr_secrets; ++i)
     free (data->secrets[i].secret);
   free (data->secrets);
-- 
2.19.0.rc0

On Thursday, 6 December 2018 16:47:32 CET Richard W.M. Jones
wrote:
> We've been carrying this exact patch in RHEL 7 for several years.  It
> reverts the change made in 2014 where we switched to using the virbr0
> bridge for libguestfs networking instead of SLIRP.  We thought SLIRP
> was going to become unsupported in qemu, but recently there have been
> more encouraging signs since it looks like SLIRP will be spun off as a
> separate project, running as a modular process and properly secured
> and supported.
> 
> This reverts commit 224de20b9a8d5ea56f6337f19b4ca237bb88eca0.
> ---
I gave this patch a try for a while, and it seems working fine so far.
Maybe it is too close to 1.30, though?

-- 
Pino Toscano