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: > 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(a)redhat.com&gt; > > --- > > 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).