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

[Martin Kletzander]

On Mon, Jun 30, 2014 at 02:28:54PM +0100, Daniel P. Berrange wrote:
>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
>

If the user updates from QEMU without DRIVE_READONLY to newer one,
that supports that flag, than XML with readonly IDE HDD will stop
working even though it worked before the update *as requested*.  That
readonly flag does not reflect how the disk is exposed in the guest;
as you said IDE does not have any readonly concept, that is only how
the device reacts to writes.

Changing the behaviour to:

if (disk->readonly &&
    virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY) &&
    !(disk->bus == VIR_DOMAIN_DISK_BUS_IDE &&
      disk->device == VIR_DOMAIN_DISK_DEVICE_DISK))
    virBufferAddLit(&opt, ",readonly=on");


would keep the backward compatibility.  This behaviour makes more
sense to me.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140630/78253464/attachment-0001.sig>