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

[Peter Krempa]

On Fri, Sep 10, 2021 at 11:18:20 -0400, 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 at redhat.com>
> > > ---
> > >   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

I strongly disagree with this statement. Firstly we now have user
aliases and secondly there's always runtime related stuff in the XML.

> 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).

Picking config XML is semantically as wrong as generating a partial XML.
It can simply be a completely different device.

If we want the users to use the full XML when updating the device it
_must_ be the full XML snippet from the running config. Otherwise we are
back at a "partial XML" and all the problems connected to that.