Libguestfs - Jul 2010 - [PATCH] Revert "add_drive_ro adds readonly=on option if available." (RHBZ#617200).

-- 
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 004c0345d6fd9f141978f5163e053a67dba31cd0 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Thu, 22 Jul 2010 14:39:36 +0100
Subject: [PATCH] Revert "add_drive_ro adds readonly=on option if
available." (RHBZ#617200).

Adding the readonly=on option is not so clever.  This causes
qemu to present the disk as read-only to the guest.  (The
expected behaviour of snapshots=on,readonly=on was that it
would open the disk O_RDONLY but present a writable disk to
the guest).

Since the guest sees a read-only disk, we are unable to do any
recovery if a filesystem on the disk is inconsistent.  This basically
prevents most accesses to live disk images.

What we really want is a qemu option which presents a writable
disk to the guest, but only opens the disk on the host side with
O_RDONLY, to alleviate the udev bug RHBZ#571714.

This reverts commit 676462684e05dd8341dd695762dd99a87d8ec022.
---
 src/generator.ml |    4 +---
 src/guestfs.c    |   22 ++++------------------
 2 files changed, 5 insertions(+), 21 deletions(-)

diff --git a/src/generator.ml b/src/generator.ml
index 3af673d..ccbc13d 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -559,15 +559,13 @@ 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,readonly=on,if=...>.
+C<-drive file=filename,snapshot=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 85a042a..d6c8d60 100644
--- a/src/guestfs.c
+++ b/src/guestfs.c
@@ -836,6 +836,9 @@ 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;
@@ -846,24 +849,7 @@ guestfs__add_drive_ro_with_if (guestfs_h *g, const char
*filename,
     return -1;
   }
 
-  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);
+  snprintf (buf, len, "file=%s,snapshot=on,if=%s", filename,
drive_if);
 
   return guestfs__config (g, "-drive", buf);
 }
-- 
1.7.1

On 22/07/10 14:46, Richard W.M. Jones wrote:
>> From 004c0345d6fd9f141978f5163e053a67dba31cd0 Mon Sep 17 00:00:00 2001
> From: Richard Jones<rjones at redhat.com>
> Date: Thu, 22 Jul 2010 14:39:36 +0100
> Subject: [PATCH] Revert "add_drive_ro adds readonly=on option if
available." (RHBZ#617200).
>
> Adding the readonly=on option is not so clever.  This causes
> qemu to present the disk as read-only to the guest.  (The
> expected behaviour of snapshots=on,readonly=on was that it
> would open the disk O_RDONLY but present a writable disk to
> the guest).
>
> Since the guest sees a read-only disk, we are unable to do any
> recovery if a filesystem on the disk is inconsistent.  This basically
> prevents most accesses to live disk images.
>
> What we really want is a qemu option which presents a writable
> disk to the guest, but only opens the disk on the host side with
> O_RDONLY, to alleviate the udev bug RHBZ#571714.
>
> This reverts commit 676462684e05dd8341dd695762dd99a87d8ec022.
> ---
>   src/generator.ml |    4 +---
>   src/guestfs.c    |   22 ++++------------------
>   2 files changed, 5 insertions(+), 21 deletions(-)
>
...
> diff --git a/src/guestfs.c b/src/guestfs.c
> index 85a042a..d6c8d60 100644
> --- a/src/guestfs.c
> +++ b/src/guestfs.c
> @@ -836,6 +836,9 @@ 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;
Could you please move these 2 lines...
> @@ -846,24 +849,7 @@ guestfs__add_drive_ro_with_if (guestfs_h *g, const
char *filename,
>       return -1;
>     }
>
> -  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);
... down here?
> +  snprintf (buf, len, "file=%s,snapshot=on,if=%s", filename,
drive_if);
>
>     return guestfs__config (g, "-drive", buf);
>   }
Apart from that, ACK. FWIW, I've also cogitated over the qemu man page 
definition of what snapshot does, and as long as we can never send 
"Ctrl-a s" to the qemu process this seems to be safe.

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

GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490