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

[Laine Stump]

On 9/13/21 11:38 AM, Peter Krempa wrote:
> 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.

Yeah, I covered the user-specified aliases in my response. 
user-specified aliases are the only reason that code is in there at all.

I did a bit of evening spelunking and found this:

1) originally the alias was completely ignored in device-update, since 
it was always set automatically by libvirt, and so it was a read-only 
attribute from the application's PoV.

2) Then user-specified aliases were added, with no associated change to 
the update-device code, and QE testing found that a requested change to 
alias during live device update was silently ignored:

    https://bugzilla.redhat.com/1585108

3) This led to a patch that checked for any change in alias in the 
update-device XML, with no special case for "not specified" (i.e. 
basically what you're suggesting the code should be):

 
https://gitlab.com/libvirt/libvirt/-/commit/4ad54a417a1bbe10aeffcce783f4381d8d43f799

4) That patch resulted in a bug filed by RHV saying that the behavior of 
the update-device API had changed, which broke their existing code for 
changing the media in a CD drive or something like that (they don't read 
back the normalized XML after they've started the guest, so they don't 
have any info about auto-generated values, and thus weren't including 
alias in the XML they sent to update-device (this was before RHV started 
specifying their own aliases):

    https://bugzilla.redhat.com/1621910

5) *That* bug resulted in the code being changed to what it is today, 
with this patch:

 
https://gitlab.com/libvirt/libvirt/-/commit/b48d9e939bcf32a8d6e571313637e2eefe52e117

So essentially we are trapped into this "ignore unspecified alias" 
semantic because alias previously existed but wasn't user-specified, and 
suddenly requiring it broke existing applications trying to do things 
completely unrelated to aliases.

TL;NMFTG (Too Long; No More ... To Give) - As with many warts in 
libvirt, I think we need to leave the check for alias as it is.

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

Unfortunately I think the train has long ago left the station on this 
(used that metaphor just for you :-))(BTW, KVM Forum was just no fun at 
all this year without a talk like the one a few years ago about using 
virtualization on locomotives.)

Anyway, based on Michal's ACK, I pushed my check for ACPI index, which 
*doesn't* ignore in the case that it's unspecified. As with all other 
attributes that are only present if they are user-specified, I think 
that is the correct behavior. (Feel free to joust with the "unspecified 
alias" windmill as much as you like though :-P)