Libguestfs - Apr 2017 - Re: [PATCH] lib: direct: Remove support for virtio-blk as the default.

virtio-scsi has been supported in qemu since 2012, and it is superior
in every respect to virtio-blk.  There's no reason to still be using
virtio-blk.

virtio-scsi support was initially added in 2012
(commit 0c0a7d0d868d153adf0600189f771459e1068b0a).

You can still use virtio-blk using the (deprecated) iface parameter,
but don't do that in new code.
---
 lib/guestfs-internal.h |  1 -
 lib/launch-direct.c    | 87 ++++++++++++++------------------------------------
 lib/qemu.c             | 47 ---------------------------
 3 files changed, 24 insertions(+), 111 deletions(-)

diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 5e9d97c07..a04ccff09 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -983,7 +983,6 @@ struct qemu_data;
 extern struct qemu_data *guestfs_int_test_qemu (guestfs_h *g, struct version
*qemu_version);
 extern int guestfs_int_qemu_supports (guestfs_h *g, const struct qemu_data *,
const char *option);
 extern int guestfs_int_qemu_supports_device (guestfs_h *g, const struct
qemu_data *, const char *device_name);
-extern int guestfs_int_qemu_supports_virtio_scsi (guestfs_h *g, struct
qemu_data *, const struct version *qemu_version);
 extern char *guestfs_int_drive_source_qemu_param (guestfs_h *g, const struct
drive_source *src);
 extern bool guestfs_int_discard_possible (guestfs_h *g, struct drive *drv,
const struct version *qemu_version);
 extern char *guestfs_int_qemu_escape_param (guestfs_h *g, const char *param);
diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index db9b9f3ef..b0038c6a9 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -60,7 +60,7 @@ struct backend_direct_data {
 };
 
 static int is_openable (guestfs_h *g, const char *path, int flags);
-static char *make_appliance_dev (guestfs_h *g, int virtio_scsi);
+static char *make_appliance_dev (guestfs_h *g);
 static void print_qemu_command_line (guestfs_h *g, char **argv);
 
 static char *
@@ -234,7 +234,6 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   CLEANUP_FREE void *buf = NULL;
   struct drive *drv;
   size_t i;
-  int virtio_scsi;
   struct hv_param *hp;
   bool has_kvm;
   int force_tcg;
@@ -336,14 +335,10 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   ADD_CMDLINE (g->hv);
 
   /* CVE-2011-4127 mitigation: Disable SCSI ioctls on virtio-blk
-   * devices.  The -global option must exist, but you can pass any
-   * strings to it so we don't need to check for the specific virtio
-   * feature.
+   * devices.
    */
-  if (guestfs_int_qemu_supports (g, data->qemu_data, "-global")) {
-    ADD_CMDLINE ("-global");
-    ADD_CMDLINE (VIRTIO_BLK ".scsi=off");
-  }
+  ADD_CMDLINE ("-global");
+  ADD_CMDLINE (VIRTIO_BLK ".scsi=off");
 
   if (guestfs_int_qemu_supports (g, data->qemu_data,
"-nodefconfig"))
     ADD_CMDLINE ("-nodefconfig");
@@ -461,16 +456,11 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
     ADD_CMDLINE ("virtio-rng-pci,rng=rng0");
   }
 
-  /* Add drives */
-  virtio_scsi = guestfs_int_qemu_supports_virtio_scsi (g, data->qemu_data,
-                                                      
&data->qemu_version);
-
-  if (virtio_scsi) {
-    /* Create the virtio-scsi bus. */
-    ADD_CMDLINE ("-device");
-    ADD_CMDLINE (VIRTIO_SCSI ",id=scsi");
-  }
+  /* Create the virtio-scsi bus. */
+  ADD_CMDLINE ("-device");
+  ADD_CMDLINE (VIRTIO_SCSI ",id=scsi");
 
+  /* Add drives */
   ITER_DRIVES (g, i, drv) {
     CLEANUP_FREE char *file = NULL, *escaped_file = NULL, *param = NULL;
 
@@ -529,10 +519,14 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
     }
 
     /* If there's an explicit 'iface', use it.  Otherwise default
to
-     * virtio-scsi if available.  Otherwise default to virtio-blk.
+     * virtio-scsi.
      */
-    if (drv->iface && STREQ (drv->iface, "virtio")) /*
virtio-blk */
-      goto virtio_blk;
+    if (drv->iface && STREQ (drv->iface, "virtio")) {
/* virtio-blk */
+      ADD_CMDLINE ("-drive");
+      ADD_CMDLINE_PRINTF ("%s,if=none" /* sic */, param);
+      ADD_CMDLINE ("-device");
+      ADD_CMDLINE_PRINTF (VIRTIO_BLK ",drive=hd%zu", i);
+    }
 #if defined(__arm__) || defined(__aarch64__) || defined(__powerpc__)
     else if (drv->iface && STREQ (drv->iface, "ide")) {
       error (g, "'ide' interface does not work on ARM or
PowerPC");
@@ -543,19 +537,12 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
       ADD_CMDLINE ("-drive");
       ADD_CMDLINE_PRINTF ("%s,if=%s", param, drv->iface);
     }
-    else if (virtio_scsi) {
+    else /* virtio-scsi */ {
       ADD_CMDLINE ("-drive");
       ADD_CMDLINE_PRINTF ("%s,if=none" /* sic */, param);
       ADD_CMDLINE ("-device");
       ADD_CMDLINE_PRINTF ("scsi-hd,drive=hd%zu", i);
     }
-    else {
-    virtio_blk:
-      ADD_CMDLINE ("-drive");
-      ADD_CMDLINE_PRINTF ("%s,if=none" /* sic */, param);
-      ADD_CMDLINE ("-device");
-      ADD_CMDLINE_PRINTF (VIRTIO_BLK ",drive=hd%zu", i);
-    }
   }
 
   /* Add the ext2 appliance drive (after all the drives). */
@@ -565,16 +552,10 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
                         "cache=unsafe,if=none,format=raw",
                         appliance);
 
-    if (virtio_scsi) {
-      ADD_CMDLINE ("-device");
-      ADD_CMDLINE ("scsi-hd,drive=appliance");
-    }
-    else {
-      ADD_CMDLINE ("-device");
-      ADD_CMDLINE (VIRTIO_BLK ",drive=appliance");
-    }
+    ADD_CMDLINE ("-device");
+    ADD_CMDLINE ("scsi-hd,drive=appliance");
 
-    appliance_dev = make_appliance_dev (g, virtio_scsi);
+    appliance_dev = make_appliance_dev (g);
   }
 
   /* Create the virtio serial bus. */
@@ -881,25 +862,18 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
  * NULL, "ide" or "virtio" (which means virtio-blk).  See
RHBZ#975797.
  */
 static char *
-make_appliance_dev (guestfs_h *g, int virtio_scsi)
+make_appliance_dev (guestfs_h *g)
 {
   size_t i, index = 0;
   struct drive *drv;
-  char dev[64] = "/dev/Xd";
+  char dev[64] = "/dev/sd";
 
   /* Calculate the index of the drive. */
   ITER_DRIVES (g, i, drv) {
-    if (virtio_scsi) {
-      if (drv->iface == NULL || STREQ (drv->iface, "ide"))
-        index++;
-    }
-    else /* virtio-blk */ {
-      if (drv->iface == NULL || STRNEQ (drv->iface, "virtio"))
-        index++;
-    }
+    if (drv->iface == NULL || STREQ (drv->iface, "ide"))
+      index++;
   }
 
-  dev[5] = virtio_scsi ? 's' : 'v';
   guestfs_int_drive_name (index, &dev[7]);
 
   return safe_strdup (g, dev);  /* Caller frees. */
@@ -1012,20 +986,7 @@ get_pid_direct (guestfs_h *g, void *datav)
 static int
 max_disks_direct (guestfs_h *g, void *datav)
 {
-  struct backend_direct_data *data = datav;
-
-  /* Get qemu help text and version. */
-  if (data->qemu_data == NULL) {
-    data->qemu_data = guestfs_int_test_qemu (g, &data->qemu_version);
-    if (data->qemu_data == NULL)
-      return -1;
-  }
-
-  if (guestfs_int_qemu_supports_virtio_scsi (g, data->qemu_data,
-                                             &data->qemu_version))
-    return 255;
-  else
-    return 27;                  /* conservative estimate */
+  return 255;
 }
 
 static struct backend_ops backend_direct_ops = {
diff --git a/lib/qemu.c b/lib/qemu.c
index 6d8fe3594..8bec6bab0 100644
--- a/lib/qemu.c
+++ b/lib/qemu.c
@@ -46,9 +46,6 @@
 struct qemu_data {
   char *qemu_help;              /* Output of qemu -help. */
   char *qemu_devices;           /* Output of qemu -device ? */
-
-  int virtio_scsi;              /* See function
-                                   guestfs_int_qemu_supports_virtio_scsi */
 };
 
 static int test_qemu (guestfs_h *g, struct qemu_data *data, struct version
*qemu_version);
@@ -303,50 +300,6 @@ guestfs_int_qemu_supports_device (guestfs_h *g,
   return strstr (data->qemu_devices, device_name) != NULL;
 }
 
-static int
-old_or_broken_virtio_scsi (const struct version *qemu_version)
-{
-  /* qemu 1.1 claims to support virtio-scsi but in reality it's broken. */
-  if (!guestfs_int_version_ge (qemu_version, 1, 2, 0))
-    return 1;
-
-  return 0;
-}
-
-/**
- * Test if qemu supports virtio-scsi.
- *
- * Returns C<1> = use virtio-scsi, or C<0> = use virtio-blk.
- */
-int
-guestfs_int_qemu_supports_virtio_scsi (guestfs_h *g, struct qemu_data *data,
-                                       const struct version *qemu_version)
-{
-  int r;
-
-  /* data->virtio_scsi has these values:
-   *   0 = untested (after handle creation)
-   *   1 = supported
-   *   2 = not supported (use virtio-blk)
-   *   3 = test failed (use virtio-blk)
-   */
-  if (data->virtio_scsi == 0) {
-    if (old_or_broken_virtio_scsi (qemu_version))
-      data->virtio_scsi = 2;
-    else {
-      r = guestfs_int_qemu_supports_device (g, data, VIRTIO_SCSI);
-      if (r > 0)
-        data->virtio_scsi = 1;
-      else if (r == 0)
-        data->virtio_scsi = 2;
-      else
-        data->virtio_scsi = 3;
-    }
-  }
-
-  return data->virtio_scsi == 1;
-}
-
 /**
  * Escape a qemu parameter.
  *
-- 
2.12.0

On Wednesday, 19 April 2017 12:00:17 CEST Richard W.M. Jones
wrote:
> virtio-scsi has been supported in qemu since 2012, and it is superior
> in every respect to virtio-blk.  There's no reason to still be using
> virtio-blk.
> 
> virtio-scsi support was initially added in 2012
> (commit 0c0a7d0d868d153adf0600189f771459e1068b0a).
> 
> You can still use virtio-blk using the (deprecated) iface parameter,
> but don't do that in new code.
> ---
LGTM, just one note below.
>    /* CVE-2011-4127 mitigation: Disable SCSI ioctls on virtio-blk
> -   * devices.  The -global option must exist, but you can pass any
> -   * strings to it so we don't need to check for the specific virtio
> -   * feature.
> +   * devices.
>     */
> -  if (guestfs_int_qemu_supports (g, data->qemu_data,
"-global")) {
> -    ADD_CMDLINE ("-global");
> -    ADD_CMDLINE (VIRTIO_BLK ".scsi=off");
> -  }
> +  ADD_CMDLINE ("-global");
> +  ADD_CMDLINE (VIRTIO_BLK ".scsi=off");
Before them, a "safety" check like the (untested) following could be
added:

  if (guestfs_int_qemu_supports_device (g, data, VIRTIO_SCSI) != 1) {
    error (g, _("QEMU with virtio-scsi support is required"));
    goto cleanup0;
  }

-- 
Pino Toscano

On Thu, Apr 20, 2017 at 02:27:45PM +0200, Pino Toscano
wrote:
> On Wednesday, 19 April 2017 12:00:17 CEST Richard W.M. Jones wrote:
> > virtio-scsi has been supported in qemu since 2012, and it is superior
> > in every respect to virtio-blk.  There's no reason to still be
using
> > virtio-blk.
> > 
> > virtio-scsi support was initially added in 2012
> > (commit 0c0a7d0d868d153adf0600189f771459e1068b0a).
> > 
> > You can still use virtio-blk using the (deprecated) iface parameter,
> > but don't do that in new code.
> > ---
> 
> LGTM, just one note below.
> 
> >    /* CVE-2011-4127 mitigation: Disable SCSI ioctls on virtio-blk
> > -   * devices.  The -global option must exist, but you can pass any
> > -   * strings to it so we don't need to check for the specific
virtio
> > -   * feature.
> > +   * devices.
> >     */
> > -  if (guestfs_int_qemu_supports (g, data->qemu_data,
"-global")) {
> > -    ADD_CMDLINE ("-global");
> > -    ADD_CMDLINE (VIRTIO_BLK ".scsi=off");
> > -  }
> > +  ADD_CMDLINE ("-global");
> > +  ADD_CMDLINE (VIRTIO_BLK ".scsi=off");
> 
> Before them, a "safety" check like the (untested) following could
be
> added:
> 
>   if (guestfs_int_qemu_supports_device (g, data, VIRTIO_SCSI) != 1) {
>     error (g, _("QEMU with virtio-scsi support is required"));
>     goto cleanup0;
>   }
I wonder if there's a case where virtio-scsi wouldn't be available.

I'm thinking if qemu modularized more hardware?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html