[libvirt] [PATCH] qemu: raise an error when trying to use readonly ide disks

[Daniel P. Berrange]

On Mon, Jun 30, 2014 at 03:23:40PM +0200, Giuseppe Scrivano wrote:
> Martin Kletzander <mkletzan at redhat.com> writes:
> 
> Q> On Mon, Jun 30, 2014 at 12:05:06PM +0200, Giuseppe Scrivano wrote:
> >>The IDE bus doesn't support readonly disks, so inform the user with an
> >>error message instead of let qemu fail with a more obscure "Device
> >>'ide-hd' could not be initialized" error message.
> >>
> >>Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1112939
> >>
> >>Signed-off-by: Giuseppe Scrivano <gscrivan at redhat.com>
> >>---
> >> src/qemu/qemu_command.c | 9 ++++++++-
> >> 1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >>index 63f322a..4829176 100644
> >>--- a/src/qemu/qemu_command.c
> >>+++ b/src/qemu/qemu_command.c
> >>@@ -3385,8 +3385,15 @@ qemuBuildDriveStr(virConnectPtr conn,
> >>         disk->bus != VIR_DOMAIN_DISK_BUS_IDE)
> >>         virBufferAddLit(&opt, ",boot=on");
> >>     if (disk->readonly &&
> >>-        virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY))
> >>+        virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY)) {
> >>+        if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE &&
> >>+            disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> >>+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >>+                           _("readonly ide disks are not supported"));
> >>+            goto error;
> >>+        }
> >>         virBufferAddLit(&opt, ",readonly=on");
> >>+    }
> >
> > I'm not very sure we should do that.  Second opinion would be great.
> > My point is that if qemu does not support the readonly flag, we just
> > skip setting it, but if it supports it, we will error out?  OTOH
> > skipping the readonly=on when user requests it is, ehm, not nice
> > either.  I think Eric was saying that *some* readonly flags do not
> > make much of a sense, but that was probably WRT backing chains or
> > something like that.  Eric?
> 
> I don't think that this error is related to the presence of the
> DRIVE_READONLY Qemu caps.  My understanding of this comment on the Qemu
> bug conterpart (https://bugzilla.redhat.com/show_bug.cgi?id=915162#c1) is
> that this is not going to change as readonly disks are not supported by
> the IDE bus, and this can be considered just an early detection of this
> situation.

Yep, that matches my understanding - IDE hardware simply doesn't have a
concept of a readonly hard disk.

The libvirt <readonly/> flag does two things

 - Causes SELinux to make the file readonly
 - Passes readonly=on to QEMU

If we silently skipped adding readonly=on, then libvirt would still tell
SELinux to make the file readonly, and then when the guest tried to issue
a write request to the disk, it would get an I/O error. Some might argue
that they want this behaviour, but I think erroring out by default is
probably better. If people really want a read-write disk in the guest
with readonly SELinux labelling, the mgmt app can always override the
<seclabel> for that disk to set a readonly selinux label.

IOW, ACK

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|