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.
