[Libguestfs] [PATCH] Revert "add_drive_ro adds readonly=on option if available." (RHBZ#617200).
Matthew Booth
mbooth at redhat.com
Thu Jul 22 15:23:13 UTC 2010
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
More information about the Libguestfs
mailing list