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