Re: [libvirt PATCH 2/2] conf: log error on attempts to modify ACPI index of active device

On 9/10/21 3:30 AM, Peter Krempa wrote: 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.