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

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(a)redhat.com&gt; >> --- >> 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.