On 9/10/21 5:18 PM, Laine Stump wrote: > On 9/10/21 3:30 AM, Peter Krempa wrote: >> On Thu, Sep 09, 2021 at 13:46:41 -0400, Laine Stump wrote: >>> The ACPI index of a device in a running guest can't be modified, and >>> libvirt doesn't actually attempt to modify it, but it was possible for >>> a user to request such a modification, and libvirt wouldn't complain, >>> thus misleading the user into thinking that it had actually been >>> changed. >>> >>> Resolves: https://bugzilla.redhat.com/1998920 >>> >>> Signed-off-by: Laine Stump <laine@xxxxxxxxxx> >>> --- >>> src/conf/domain_conf.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >>> index fefc527901..7ff890d8b7 100644 >>> --- a/src/conf/domain_conf.c >>> +++ b/src/conf/domain_conf.c >>> @@ -28485,6 +28485,12 @@ virDomainDefCompatibleDevice(virDomainDef *def, >>> _("changing device alias is not allowed")); >>> return -1; >>> } >>> + >>> + if (data.newInfo->acpiIndex != data.oldInfo->acpiIndex) { >>> + virReportError(VIR_ERR_OPERATION_DENIED, "%s", >>> + _("changing device 'acpi index' is not >>> allowed")); >>> + return -1; >>> + } >> >> I don't remember any more what the intended semantics of the 'update' >> API are in regards to asking the user to provide a complete definition >> of the device, but according to the 'alias' check above we seem to be >> specifically ignoring the situation when the new definition didn't >> provide an alias. This seems to be hinting that we ignore stuff that the >> used didn't provide. >> >> If that's the case you'll need to specifically exclude index '0' from >> the above check. > > The difference here is that alias is something that normally is not set > by the user, but is instead auto-generated by libvirt when the guest > starts and not stored/reported in the persistent config. So in the > common case of "grab config XML, extract device element, modify it, send > it to update-device" the alias wouldn't be in the XML, but if the user > instead grabs the *status* XML then the alias *would* be there. > (However, if the alias had been set by the user (with the prefix "ua-"), > the alias would be present in the config XML.) So we allow for either > blank (in the case that config XML was used) or no-change (in case alias > was user-set and/or the status XML is used). > > The acpi index, on the other hand, is always unset unless the user > specifies it, and so is never auto-generated and always matches in both > the config and status XMLs. So whichever way the user gets the original > XML (status or config) they are always going to have the actual setting. > > There are many other attributes that are checked to assure they match in > (e.g.) qemuDomainChangeNet() (for example, network device model name, > virtio options, upscript name,...) and (with one exception) none of > those check for "not specified or not changed" - they all just check for > "not changed". The one exception is ifname, which is another attribute > that (like alias) is autogenerated every time the guest starts, and is > never output in config XML (unless it was user-specified); for ifname if > it's been left unspecified in the update XML, then the value from the > status is copied over, making it also effectively ("unspecified or > unchanged"). But again, this special handling isn't done for any > attribute that isn't both auto-generated and ephemeral (as alias and > ifname are). > > So in the end, I think it's correct that validation of acpi index > *shouldn't* ignore an "apparently missing" value in the update. > > ====== > > BTW, something forgot to mention in the commit log - in the case of, > e.g., network interfaces which is the only type of device where an alias > is useful, most of the checks are in qemuDomainChangeNet() rather than > in this virDomainDefCompatibleDevice() function. I was originally going > to add this check into qemuDomainChangeNet() as well, but it is an > attribute that's technically valid for any PCI device, not just > interfaces; searching around, I found that > virDomainDefCompatibleDevice() is called on update for all devices, and > is already used to pretect against alias change (another attribute > that's valid for all devices), so in the interest of consistency I put > the check there. > > That said, although I used it based on precendent, I can't say that I > really like virDomainDefCompatibleDevice(). It is used for multiple > unrelated things - the bit at the beginning of the function is only > executed if action is UPDATE and live is true, and all the rest of the > function is only useful if action is ATTACH - it's checking things that > could verifiably never happen during a live device update (e.g. checking > if the device is USB but the domain doesn't support USB devices, or > checking attributes for a device type that isn't allowed to be updated > at all.). So it's really 2 completely independent functions in one, > where the final two args act as an extension to the function name. > > I notice its action and live args previously weren't there - I did the > spelunking into git history to find when/how; essentially there was at > one time an unused action arg which was removed, but then re-added in > order to add the check for alias). I didn't dig much deeper, but it > seems like this might be an unnecessary tangle that should be split up > into multiple functions. I didn't want to mess with that for a simple > bugfix, though. > Thank you Laine for your detailed analysis. I think you can merge this patch. Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx> Michal
