[libvirt] [PATCH] qemuDomainDiskChangeSupported: Forbid alias change
Since we have user aliases it may happen that users want to change it using 'update-device'. Instead of ignoring it silently, error out loudly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 347fc0742..00d6c41c9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6762,6 +6762,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, return false; } + if (disk->info.alias && + STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "alias"); + return false; + } + CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); CHECK_EQ(sgio, "sgio", true); -- 2.13.6
On 12/13/2017 03:30 PM, Michal Privoznik wrote:
Since we have user aliases it may happen that users want to change it using 'update-device'. Instead of ignoring it silently, error out loudly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++++++ 1 file changed, 8 insertions(+)
Ping. Michal
On 12/13/2017 09:30 AM, Michal Privoznik wrote:
Since we have user aliases it may happen that users want to change it using 'update-device'. Instead of ignoring it silently, error out loudly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++++++ 1 file changed, 8 insertions(+)
So IIUC this would be adding a check that the libvirt supplied alias matches, true? But your goal is to check if a "ua-" prefixed alias was being used - so should the check use of the prefix? There's probably a few more info. fields that could be considered - any thoughts on those? Perhaps fairly difficult to keep up with the various _virDomainDiskDef and friends alterations for what this function is indicating... John
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 347fc0742..00d6c41c9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6762,6 +6762,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, return false; }
+ if (disk->info.alias && + STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "alias"); + return false; + } + CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); CHECK_EQ(sgio, "sgio", true);
On 01/05/2018 01:15 AM, John Ferlan wrote:
On 12/13/2017 09:30 AM, Michal Privoznik wrote:
Since we have user aliases it may happen that users want to change it using 'update-device'. Instead of ignoring it silently, error out loudly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++++++ 1 file changed, 8 insertions(+)
So IIUC this would be adding a check that the libvirt supplied alias matches, true? But your goal is to check if a "ua-" prefixed alias was being used - so should the check use of the prefix?
I don't think this is limited to user provided aliases only. For instance, libvirt generates the following aliases for disks: scsi0-0-0, ide0-1-0, fdc0-0-0. Now, what should happen if an user provides new disk XML trying to change nothing but 'scsi0-0-0' to say 'scsi0-0-1' (for live domain)? I think it should be forbidden - we don't have way to change IDs in qemu, do we? We certainly lack implementation to support it. Therefore, I think we don't have to limit the check just for 'ua-' prefixes.
There's probably a few more info. fields that could be considered - any thoughts on those? Perhaps fairly difficult to keep up with the various _virDomainDiskDef and friends alterations for what this function is indicating...
Yeah, probably. I'm not sure. Maybe we will find some more. Michal
On 01/05/2018 03:20 AM, Michal Privoznik wrote:
On 01/05/2018 01:15 AM, John Ferlan wrote:
On 12/13/2017 09:30 AM, Michal Privoznik wrote:
Since we have user aliases it may happen that users want to change it using 'update-device'. Instead of ignoring it silently, error out loudly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++++++ 1 file changed, 8 insertions(+)
So IIUC this would be adding a check that the libvirt supplied alias matches, true? But your goal is to check if a "ua-" prefixed alias was being used - so should the check use of the prefix?
I don't think this is limited to user provided aliases only. For instance, libvirt generates the following aliases for disks: scsi0-0-0, ide0-1-0, fdc0-0-0.
Now, what should happen if an user provides new disk XML trying to change nothing but 'scsi0-0-0' to say 'scsi0-0-1' (for live domain)? I think it should be forbidden - we don't have way to change IDs in qemu, do we? We certainly lack implementation to support it.
Therefore, I think we don't have to limit the check just for 'ua-' prefixes.
OK - fair enough... Perhaps the commit message is what caught my attention/focus to ua- in particular. Maybe the extra explanation here can be worked in. Reviewed-by: John Ferlan <jferlan@redhat.com> John
There's probably a few more info. fields that could be considered - any thoughts on those? Perhaps fairly difficult to keep up with the various _virDomainDiskDef and friends alterations for what this function is indicating...
Yeah, probably. I'm not sure. Maybe we will find some more.
Michal
On Wed, Dec 13, 2017 at 03:30 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
Since we have user aliases it may happen that users want to change it using 'update-device'. Instead of ignoring it silently, error out loudly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 347fc0742..00d6c41c9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6762,6 +6762,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, return false; }
+ if (disk->info.alias && + STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "alias"); + return false; + } + CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); CHECK_EQ(sgio, "sgio", true); -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
I'm not sure if our current approach is the way to go… Couldn’t we just invert the logic for qemuDomainDiskChangeSupported in that way that it returns by default false and only for cases where we know that it’s supported to change we return true? Not sure if this proposed approach is feasible. -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 01/05/2018 01:16 PM, Marc Hartmayer wrote:
On Wed, Dec 13, 2017 at 03:30 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
Since we have user aliases it may happen that users want to change it using 'update-device'. Instead of ignoring it silently, error out loudly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 347fc0742..00d6c41c9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6762,6 +6762,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, return false; }
+ if (disk->info.alias && + STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "alias"); + return false; + } + CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); CHECK_EQ(sgio, "sgio", true); -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
I'm not sure if our current approach is the way to go… Couldn’t we just invert the logic for qemuDomainDiskChangeSupported in that way that it returns by default false and only for cases where we know that it’s supported to change we return true? Not sure if this proposed approach is feasible.
Maybe. If you want to work on it, be my guest. It looks feasible to me. Unfortunately, I'm buried under some other work right now. Michal
On Fri, Jan 05, 2018 at 02:24 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
On 01/05/2018 01:16 PM, Marc Hartmayer wrote:
On Wed, Dec 13, 2017 at 03:30 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
Since we have user aliases it may happen that users want to change it using 'update-device'. Instead of ignoring it silently, error out loudly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 347fc0742..00d6c41c9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6762,6 +6762,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, return false; }
+ if (disk->info.alias && + STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "alias"); + return false; + } + CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); CHECK_EQ(sgio, "sgio", true); -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
I'm not sure if our current approach is the way to go… Couldn’t we just invert the logic for qemuDomainDiskChangeSupported in that way that it returns by default false and only for cases where we know that it’s supported to change we return true? Not sure if this proposed approach is feasible.
Maybe. If you want to work on it, be my guest. It looks feasible to me.
Currently, I don't have time for this as it’s not that important.
Unfortunately, I'm buried under some other work right now.
Michal
-- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (3)
-
John Ferlan -
Marc Hartmayer -
Michal Privoznik