On Mon, Jun 30, 2014 at 03:23:40PM +0200, Giuseppe Scrivano wrote: > Martin Kletzander <mklet...@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 <gscri...@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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
