[libvirt] [PATCH] qemu: raise an error when trying to use readonly ide disks
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@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"); + } if (disk->transient) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("transient disks not supported yet")); -- 1.9.3
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@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? Martin
if (disk->transient) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("transient disks not supported yet")); -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Martin Kletzander <mkletzan@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@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. Regards, Giuseppe
On Mon, Jun 30, 2014 at 03:23:40PM +0200, Giuseppe Scrivano wrote:
Martin Kletzander <mkletzan@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@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 :|
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@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@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
Martin Kletzander <mkletzan@redhat.com> writes:
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.
this behaves in a quite different way that my proposed patch but if <readonly/> affects also the SELinux label and we allow the process to run anyway by skipping readonly=on for IDE disks, wouldn't qemu fail in weird ways when trying to write to the file? Regards, Giuseppe
On Tue, Jul 01, 2014 at 09:40:00AM +0200, Giuseppe Scrivano wrote:
Martin Kletzander <mkletzan@redhat.com> writes:
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.
this behaves in a quite different way that my proposed patch but if <readonly/> affects also the SELinux label and we allow the process to run anyway by skipping readonly=on for IDE disks, wouldn't qemu fail in weird ways when trying to write to the file?
My case was that it doesn't fail in a new way if you update from older qemu that didn't support the DRIVE_READONLY capability. However it would probably fail differently when there is readonly=on because it would not report an error in the guest. Anyway, thinking about it, qemu doesn't start for some versions of libvirt already with these setting, this just changes the error to a more readable one. So given the circumstances such backward compatibility is not such huge issue (since we already broke it). I, therefore, agree with Dan's ACK. Martin
participants (3)
-
Daniel P. Berrange -
Giuseppe Scrivano -
Martin Kletzander