On 08/29/2018 03:45 PM, John Ferlan wrote: > > > 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@xxxxxxxxxx> >> --- >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
