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

[Peter Krempa]

On Fri, Sep 10, 2021 at 10:57:37 +0200, Michal Prívozník wrote:
> On 9/10/21 9: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.
> > 
> 
> While I agree that it would be user friendly I think we mandate users to
> provide full device XML and looking into the doc we really do:
> 
>  * virDomainUpdateDeviceFlags:
>  * @domain: pointer to domain object
>  * @xml: pointer to XML description of one device
>  * @flags: bitwise-OR of virDomainDeviceModifyFlags
>  *
> 
>  * The supplied XML description of the device should contain all
>  * the information that is found in the corresponding domain XML.
>  * Leaving out any piece of information may be treated as a
>  * request for its removal, which may be denied.
> 
> We can special case acpiIndex as you suggest but I think that's the
> farthest we should go. Otherwise it'll be hard to distinguish user's
> laziness to provide full XML from genuine request for removal.

I definitely agree. It may be in certain cases even impossible to do the
right thing, as e.g. removing an attribute/element may be a genuine user
request to remove something.

That's what acutally surprised me in case of the 'alias' check. If we
want to go the proper way and require a full XML we should also remove
the exemption for alias.