[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] qemu: qemuDomainChangeNet: validity checks should be done before XML autocompletion



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 redhat com>
> > ---
> > 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]