Libguestfs - Sep 2015 - [PATCH] launch: libvirt: Better error when bridge / virbr0 doesn't exist (RHBZ#1262127).

The current diagnostic is terrible.  This one tells the user how to
diagnose and fix the problem.
---
 src/launch-libvirt.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index 1c0bfac..d4c4c47 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -181,6 +181,7 @@ 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);
@@ -393,6 +394,9 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
   }
   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);
 
@@ -1988,6 +1992,49 @@ 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)
 {
-- 
2.5.0

On Thursday 10 September 2015 23:07:05 Richard W.M. Jones
wrote:
> The current diagnostic is terrible.  This one tells the user how to
> diagnose and fix the problem.
> ---
>  src/launch-libvirt.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
> index 1c0bfac..d4c4c47 100644
> --- a/src/launch-libvirt.c
> +++ b/src/launch-libvirt.c
> @@ -181,6 +181,7 @@ 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);
> @@ -393,6 +394,9 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
>    }
>    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);
>  
> @@ -1988,6 +1992,49 @@ 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;
> +}
> +
I guess this will not work on non-Linux platforms, even when using the
fixed appliance. I guess that isn't critical for now, so either put the
whole content of check_bridge_exists within #ifdef __linux__ (returning
0 otherwise), or push it as it is and we will eventually deal with that
later.

Thanks,
-- 
Pino Toscano