Libguestfs - Mar 2010 - [PATCH 0/2] Add readonly=on option to qemu -drive command line

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
-------------- next part --------------
>From 5442f45aea522a728fa5b06396d4f08d8506d7de Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Tue, 16 Mar 2010 21:44:26 +0000
Subject: [PATCH 1/2] Allow qemu_supports to run earlier.

Reimplement qemu_supports() internal function.  Allow it to run
before launch so we can test qemu features.  Document that you
should run guestfs_set_qemu as early as possible to make sure
these tests are reliable.
---
 src/generator.ml |   10 +++++++++-
 src/guestfs.c    |   27 ++++++++++++++++++---------
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/src/generator.ml b/src/generator.ml
index 486b9d8..4f569ba 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -581,7 +581,15 @@ configure script.
 You can also override this by setting the C<LIBGUESTFS_QEMU>
 environment variable.
 
-Setting C<qemu> to C<NULL> restores the default qemu
binary.");
+Setting C<qemu> to C<NULL> restores the default qemu binary.
+
+Note that you should call this function as early as possible
+after creating the handle.  This is because some pre-launch
+operations depend on testing qemu features (by running C<qemu -help>).
+If the qemu binary changes, we don't retest features, and
+so you might see inconsistent results.  Using the environment
+variable C<LIBGUESTFS_QEMU> is safest of all since that picks
+the qemu binary at the same time as the handle is created.");
 
   ("get_qemu", (RConstString "qemu", []), -1, [],
    [InitNone, Always, TestRun (
diff --git a/src/guestfs.c b/src/guestfs.c
index 2ba062c..af60699 100644
--- a/src/guestfs.c
+++ b/src/guestfs.c
@@ -91,6 +91,7 @@ static int recv_from_daemon (guestfs_h *g, uint32_t *size_rtn,
void **buf_rtn);
 static int accept_from_daemon (guestfs_h *g);
 static int check_peer_euid (guestfs_h *g, int sock, uid_t *rtn);
 static void close_handles (void);
+static int qemu_supports (guestfs_h *g, const char *option);
 
 #define UNIX_PATH_MAX 108
 
@@ -870,8 +871,6 @@ dir_contains_files (const char *dir, ...)
 
 static void print_timestamped_message (guestfs_h *g, const char *fs, ...);
 static int build_supermin_appliance (guestfs_h *g, const char *path, char
**kernel, char **initrd);
-static int test_qemu (guestfs_h *g);
-static int qemu_supports (guestfs_h *g, const char *option);
 static int is_openable (guestfs_h *g, const char *path, int flags);
 static void print_cmdline (guestfs_h *g);
 
@@ -1023,7 +1022,7 @@ guestfs__launch (guestfs_h *g)
     print_timestamped_message (g, "begin testing qemu features");
 
   /* Get qemu help text and version. */
-  if (test_qemu (g) == -1)
+  if (qemu_supports (g, NULL) == -1)
     goto cleanup0;
 
   /* Choose which vmchannel implementation to use. */
@@ -1607,11 +1606,6 @@ test_qemu (guestfs_h *g)
   char cmd[1024];
   FILE *fp;
 
-  free (g->qemu_help);
-  free (g->qemu_version);
-  g->qemu_help = NULL;
-  g->qemu_version = NULL;
-
   snprintf (cmd, sizeof cmd, "LC_ALL=C '%s' -help",
g->qemu);
 
   fp = popen (cmd, "r");
@@ -1672,11 +1666,26 @@ read_all (guestfs_h *g, FILE *fp, char **ret)
 
 /* Test if option is supported by qemu command line (just by grepping
  * the help text).
+ *
+ * The first time this is used, it has to run the external qemu
+ * binary.  If that fails, it returns -1.
+ *
+ * To just do the first-time run of the qemu binary, call this with
+ * option == NULL, in which case it will return -1 if there was an
+ * error doing that.
  */
 static int
 qemu_supports (guestfs_h *g, const char *option)
 {
-  return g->qemu_help && strstr (g->qemu_help, option) != NULL;
+  if (!g->qemu_help) {
+    if (test_qemu (g) == -1)
+      return -1;
+  }
+
+  if (option == NULL)
+    return 1;
+
+  return strstr (g->qemu_help, option) != NULL;
 }
 
 /* Check if a file can be opened. */
-- 
1.7.0.1

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
-------------- next part --------------
>From 676462684e05dd8341dd695762dd99a87d8ec022 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Tue, 16 Mar 2010 21:50:13 +0000
Subject: [PATCH 2/2] add_drive_ro adds readonly=on option if available.

Change the add_drive_ro call so it adds the readonly=on option
if qemu supports that.

This just means that qemu will not try to open the drive with
O_RDWR, and should not otherwise change the behaviour of qemu or
libguestfs.  (In particular, writes to the read-only drive are
still permitted, and are just discarded when the handle is closed).

However it should alleviate RHBZ#571714 where udev was deciding
to incorrectly relabel a device because we had opened the device
for writing (even though we didn't actually write to it).
---
 src/generator.ml |    4 +++-
 src/guestfs.c    |   22 ++++++++++++++++++----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/src/generator.ml b/src/generator.ml
index 4f569ba..83f307b 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -544,13 +544,15 @@ handle is closed.  We don't currently have any method
to enable
 changes to be committed, although qemu can support this.
 
 This is equivalent to the qemu parameter
-C<-drive file=filename,snapshot=on,if=...>.
+C<-drive file=filename,snapshot=on,readonly=on,if=...>.
 
 C<if=...> is set at compile time by the configuration option
 C<./configure --with-drive-if=...>.  In the rare case where you
 might need to change this at run time, use C<guestfs_add_drive_with_if>
 or C<guestfs_add_drive_ro_with_if>.
 
+C<readonly=on> is only added where qemu supports this option.
+
 Note that this call checks for the existence of C<filename>.  This
 stops you from specifying other types of drive which are supported
 by qemu such as C<nbd:> and C<http:> URLs.  To specify those, use
diff --git a/src/guestfs.c b/src/guestfs.c
index af60699..02d5fdb 100644
--- a/src/guestfs.c
+++ b/src/guestfs.c
@@ -792,9 +792,6 @@ int
 guestfs__add_drive_ro_with_if (guestfs_h *g, const char *filename,
                                const char *drive_if)
 {
-  size_t len = strlen (filename) + 64;
-  char buf[len];
-
   if (strchr (filename, ',') != NULL) {
     error (g, _("filename cannot contain ',' (comma)
character"));
     return -1;
@@ -805,7 +802,24 @@ guestfs__add_drive_ro_with_if (guestfs_h *g, const char
*filename,
     return -1;
   }
 
-  snprintf (buf, len, "file=%s,snapshot=on,if=%s", filename,
drive_if);
+  if (qemu_supports (g, NULL) == -1)
+    return -1;
+
+  /* Only SCSI and virtio drivers support readonly mode.
+   * This is only supported as a QEMU feature since 2010/01.
+   */
+  int supports_ro = 0;
+  if ((STREQ (drive_if, "scsi") || STREQ (drive_if,
"virtio")) &&
+      qemu_supports (g, "readonly=on"))
+    supports_ro = 1;
+
+  size_t len = strlen (filename) + 100;
+  char buf[len];
+
+  snprintf (buf, len, "file=%s,snapshot=on,%sif=%s",
+            filename,
+            supports_ro ? "readonly=on," : "",
+            drive_if);
 
   return guestfs__config (g, "-drive", buf);
 }
-- 
1.7.0.1

On 16/03/10 21:52, Richard W.M. Jones wrote:
> 
I think these 2 patches lend more weight to a port of libguestfs to libvirt.

Matt
-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

M:       +44 (0)7977 267231
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490