On Thu, Aug 09, 2018 at 05:50:11PM +0200, Ján Tomko wrote: > On Thu, Aug 09, 2018 at 11:21:52AM +0200, Katerina Koukiou wrote: > > This patch ensures that changes in attributes of interfaces will be emit > > errors accept if they are missing from the XML. > > Previously we were falsely reporting successfull updates, because some > > *successful > > > changed attributes got overwritten before the validity checks. > > The more I think about this 'feature' that allows you to omit parts of > the changed XML, the weirder it gets. > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1599513 > > > > Signed-off-by: Katerina Koukiou <kkoukiou@xxxxxxxxxx> > > --- > > src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++++++---------------- > > 1 file changed, 26 insertions(+), 16 deletions(-) > > > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > > index 1488f0a7c2..76ab56a479 100644 > > --- a/src/qemu/qemu_hotplug.c > > +++ b/src/qemu/qemu_hotplug.c > > @@ -3445,23 +3445,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, > > goto cleanup; > > } > > > > - /* info: if newdev->info is empty, fill it in from olddev, > > - * otherwise verify that it matches - nothing is allowed to > > - * change. (There is no helper function to do this, so > > - * individually check the few feidls of virDomainDeviceInfo that > > - * are relevant in this case). > > + /* info: Nothing is allowed to change. First fill the missing newdev->info > > + * from olddev and then check for changes. > > Maybe the other way around (checking first - with respect to what was > specified in the XML, then overwriting) might be cleaner, see below. > > > */ > > - if (!virDomainDeviceAddressIsValid(&newdev->info, > > - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && > > - virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) { > > - goto cleanup; > > - } > > - if (!virPCIDeviceAddressEqual(&olddev->info.addr.pci, > > - &newdev->info.addr.pci)) { > > - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", > > - _("cannot modify network device guest PCI address")); > > - goto cleanup; > > - } > > /* grab alias from olddev if not set in newdev */ > > if (!newdev->info.alias && > > VIR_STRDUP(newdev->info.alias, olddev->info.alias) < 0) > > @@ -3469,26 +3455,50 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, > > > > /* device alias is checked already in virDomainDefCompatibleDevice */ > > > > + if (newdev->info.rombar == VIR_TRISTATE_BOOL_ABSENT) > > + newdev->info.rombar = olddev->info.rombar; > > if (olddev->info.rombar != newdev->info.rombar) { > > virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", > > _("cannot modify network device rom bar setting")); > > goto cleanup; > > } > > + > > + if (!newdev->info.romfile && > > + VIR_STRDUP(newdev->info.romfile, olddev->info.romfile) < 0) > > + goto cleanup; > > if (STRNEQ_NULLABLE(olddev->info.romfile, newdev->info.romfile)) { > > virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", > > _("cannot modify network rom file")); > > goto cleanup; > > } > > + > > + if (newdev->info.bootIndex == 0) > > + newdev->info.bootIndex = olddev->info.bootIndex; > > if (olddev->info.bootIndex != newdev->info.bootIndex) { > > virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", > > _("cannot modify network device boot index setting")); > > goto cleanup; > > } > > + > > + if (newdev->info.romenabled == VIR_TRISTATE_BOOL_ABSENT) > > + newdev->info.romenabled = olddev->info.romenabled; > > if (olddev->info.romenabled != newdev->info.romenabled) { > > virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", > > _("cannot modify network device rom enabled setting")); > > goto cleanup; > > } > > + > > + /* if pci addr is missing or is invalid we overwrite it from olddev */ > > + if (!virDomainDeviceAddressIsValid(&newdev->info, > > + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { > > + newdev->info.addr.pci = olddev->info.addr.pci; > > This is an insufficient way to copy the address. The 'addr' union might > be containing an address of a different type. And the 'info.type' attribute also > needs to be copied, either 1) manually or via a 2) DeviceInfoCopy call. I am going to add the type check before the addr check. Good point. > > Also, the checks can be moved to a separate fuction first (qemuDomainChangeNet > is over 400 lines long now), e.g. qemuDomainChangeNetAllowed. I am against splitting the checks and autofilling in different functions, since it would be harder to spot if someone who is doing some change changed both parts. Katerina > > Then either: > 1) change the unconditional copy of the attributes not present in newdev > to call another helper (DeviceInfoCopyIfNeeded? MaybeCopy?) that only fills > the missing parts; or > 2) Make qemuDomainChangeNetAllowed only look at specified attributes and > move the DeviceInfoCopy call after it. (Also, to prevent memory > leaks, the original info should be cleared before calling copy) > > Jano > > > + } > > + if (!virPCIDeviceAddressEqual(&olddev->info.addr.pci, > > + &newdev->info.addr.pci)) {
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
