8:16 p.m.
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(a)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/4ad54a417a1bbe10aeffcce783f43...
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/b48d9e939bcf32a8d6e571313637e...
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)