[libvirt] [PATCH] virDomainDetachDeviceFlags: Clarify update semantics
https://bugzilla.redhat.com/show_bug.cgi?id=1621910 When users want to update a path to a CDROM they tend to construct a very minimal XML and feed the API with it. This is not a good practice as it breaks the assumptions the API is built on. Most notably, leaving an element out should be treated as a request for removal of the corresponding setting. Just like leaving out <bandwidth/> clears out any QoS previously set. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-domain.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ef460277f7..bd8ca6eff2 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8318,6 +8318,14 @@ virDomainDetachDeviceFlags(virDomainPtr domain, * media, altering the graphics configuration such as password, * reconfiguring the NIC device backend connectivity, etc. * + * The supplied XML description of the device should contain all + * the information that are found in corresponding domain XML. + * Leaving out any piece of information is treated as request for + * its removal, which may be denied. For instance, when users + * want to change CDROM media only for live XML, they must + * provide live disk XML as found in corresponding live domain + * XML with only the disk path changed. + * * Returns 0 in case of success, -1 in case of failure. */ int -- 2.16.4
On Mon, Aug 27, 2018 at 03:50:20PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1621910
When users want to update a path to a CDROM they tend to construct a very minimal XML and feed the API with it. This is not a good practice as it breaks the assumptions the API is built on. Most notably, leaving an element out should be treated as a request for removal of the corresponding setting. Just like leaving out <bandwidth/> clears out any QoS previously set.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-domain.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ef460277f7..bd8ca6eff2 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8318,6 +8318,14 @@ virDomainDetachDeviceFlags(virDomainPtr domain, * media, altering the graphics configuration such as password, * reconfiguring the NIC device backend connectivity, etc. * + * The supplied XML description of the device should contain all + * the information that are found in corresponding domain XML.
s/that /that / s/are found/is found/ (information is uncountable) s/in (corresponding)/in the \1/
+ * Leaving out any piece of information is treated as request for
s/as request/as a request/
+ * its removal, which may be denied. For instance, when users + * want to change CDROM media only for live XML, they must + * provide live disk XML as found in corresponding live domain
s/in /in the/
+ * XML with only the disk path changed.
I agree that such a laziness only leads to an inconsistent user experience where libvirt has no idea whether the user is just truly lazy to supply all the necessary information and therefore we're supposed to preserve such setting or they actually want to remove the setting. Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 08/27/2018 09:50 AM, Michal Privoznik wrote:
FWIW: my thoughts w/r/t "alias" (in particular) and the bz - since libvirt creates the alias for the live XML and does not extend that alias to the config XML that means I can see just cause for the client to believe libvirt should handle supplying the alias for the update. I thought that was one of the factors behind creating "ua-" prefixes for user aliases. IOW: User supplied an alias, so use it; otherwise, use what libvirt creates. One could say something similar for <address> since we create it; however, the difference there is that the <address> is written to the config XML for the next time. While I agree in general with supplying a "robust" XML model in order to be certain that we'd be updating the "correct" device and not just the "first device" found, I'm not convinced that by just documenting it the "problem" is fixed and the continuing conversation in the bz since you posted this patch is, well, interesting. In a way it's bringing up the reasons that we (libvirt) find it difficult to ensure we're making the right update without adding "too much" code. Move the problem up the stack!
When users want to update a path to a CDROM they tend to construct a very minimal XML and feed the API with it. This is not a good practice as it breaks the assumptions the API is built on. Most notably, leaving an element out should be treated as a request for removal of the corresponding setting. Just like leaving out <bandwidth/> clears out any QoS previously set.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-domain.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ef460277f7..bd8ca6eff2 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8318,6 +8318,14 @@ virDomainDetachDeviceFlags(virDomainPtr domain, * media, altering the graphics configuration such as password, * reconfiguring the NIC device backend connectivity, etc. * + * The supplied XML description of the device should contain all + * the information that are found in corresponding domain XML.
s/that are/that is/
+ * Leaving out any piece of information is treated as request for
s/is treated/may be treated/ Wouldn't that be hypervisor dependent? Sure most seem to call virDomainDeviceDefParse with PARSE_INACTIVE | PARSE_SKIP_VALIDATE
+ * its removal, which may be denied. For instance, when users + * want to change CDROM media only for live XML, they must + * provide live disk XML as found in corresponding live domain + * XML with only the disk path changed.
Seems that's easier written than done in practice! In particular the means by which each device is uniquely identified for the live guest. For QEMU that's the alias. Is that true for other hypervisors? I think that's where the argument starts to fall apart that one must provide full XML.
+ * * Returns 0 in case of success, -1 in case of failure. */ int
John I see Eric responded too, but figured I'd leave my change on the counter too ;-)
On 08/29/2018 03:45 PM, John Ferlan wrote:
On 08/27/2018 09:50 AM, Michal Privoznik wrote:
FWIW: my thoughts w/r/t "alias" (in particular) and the bz - since libvirt creates the alias for the live XML and does not extend that alias to the config XML that means I can see just cause for the client to believe libvirt should handle supplying the alias for the update. I thought that was one of the factors behind creating "ua-" prefixes for user aliases. IOW: User supplied an alias, so use it; otherwise, use what libvirt creates. One could say something similar for <address> since we create it; however, the difference there is that the <address> is written to the config XML for the next time.
While I agree in general with supplying a "robust" XML model in order to be certain that we'd be updating the "correct" device and not just the "first device" found, I'm not convinced that by just documenting it the "problem" is fixed and the continuing conversation in the bz since you posted this patch is, well, interesting. In a way it's bringing up the reasons that we (libvirt) find it difficult to ensure we're making the right update without adding "too much" code. Move the problem up the stack!
When users want to update a path to a CDROM they tend to construct a very minimal XML and feed the API with it. This is not a good practice as it breaks the assumptions the API is built on. Most notably, leaving an element out should be treated as a request for removal of the corresponding setting. Just like leaving out <bandwidth/> clears out any QoS previously set.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-domain.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ef460277f7..bd8ca6eff2 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8318,6 +8318,14 @@ virDomainDetachDeviceFlags(virDomainPtr domain, * media, altering the graphics configuration such as password, * reconfiguring the NIC device backend connectivity, etc. * + * The supplied XML description of the device should contain all + * the information that are found in corresponding domain XML.
s/that are/that is/
+ * Leaving out any piece of information is treated as request for
s/is treated/may be treated/
Wouldn't that be hypervisor dependent? Sure most seem to call virDomainDeviceDefParse with PARSE_INACTIVE | PARSE_SKIP_VALIDATE
That's problem of the drivers that they don't parse all the info user gave them. It is very likely a bug. I vaguely recall some changes done to qemu driver so that is parses the device XML with respect to the domain state (if change to live domain was requested the device XML is parsed as live too).
+ * its removal, which may be denied. For instance, when users + * want to change CDROM media only for live XML, they must + * provide live disk XML as found in corresponding live domain + * XML with only the disk path changed.
Seems that's easier written than done in practice! In particular the means by which each device is uniquely identified for the live guest.
Users can hardly know what we use to identify the device. In cases of disks it is currently the target string (this is true across all drivers) but this might change. Yes, our matching of device for this particular API is broken by design.
For QEMU that's the alias.
Actually it is not.
Is that true for other hypervisors? I think that's where the argument starts to fall apart that one must provide full XML.
BTW: I've posted a patch that exempts alias from this requirement I'm adding: https://www.redhat.com/archives/libvir-list/2018-August/msg01858.html However, it doesn't invalidate this requirement, it's just me trying to be nice to users and allow them to use their (broken) approach. Michal
participants (3)
-
Erik Skultety -
John Ferlan -
Michal Privoznik