[libvirt] [PATCH] virDomainDetachDeviceFlags: Clarify update semantics

[John Ferlan]

On 08/27/2018 09:50 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1621910
> 

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 at 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 ;-)