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@xxxxxxxxxx>
---
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)
