[libvirt] [PATCH] hostdev: fix two bugs in virHostdevReAttachPCIDevices

Huanle Han hanxueluo at gmail.com
Wed Mar 25 16:04:44 UTC 2015


I'm sorry that I didn't configure the 'git send-email' well.
The 2 issues can be consider separate.
Issue 1(move the 'i++'):
Yes, I do mean that we shouldn't update the 'i' value for the reason you
said. I was thinking to minimize the change. But while loop is also a good
choose.

Issue 2:
Steps to reproduce the bug :
1. Domain A and B xml contain the same SRIOV net hostdev(same pci address).
'    <interface type='hostdev' managed='yes'>
      <source>
        <address type='pci' domain='0x0000' bus='0x00' slot='0x07'
function='0x0'/>
      </source>
      <mac address='52:11:11:22:22:22'/>
    </interface>'
2. virsh start A (Successfully, and configure the SRIOV net mac witch
custom one )
3. virsh start B (Fail because the hostdev is used by domain A or other
reason.)
The failure in step 3 makes the mac/vlan of the net in Doamin A change.
Because when we call 'virHostdevReAttachPCIDevices' for domain B in step 3,
we do 'virHostdevNetConfigRestore' for all net hostdev, even the one is
being used by Domain A. The mac/vlan of the SRIOV net back to the value
before step 2.

In my fix, as the pci used by other domain have been removed from 'pcidevs'
in loop 1,
 we only need do 'virHostdevNetConfigRestore' for the hostdev still in
'pcidevs'(used by current domain).

I will resend a patch. Thanks for your suggestion.


2015-03-24 20:47 GMT+08:00 John Ferlan <jferlan at redhat.com>:

>
>
> On 03/22/2015 09:59 AM, Huanle Han wrote:
> >
> > Bug 1: The the next element in the pcidevs is skipped after we
> > virPCIDeviceListDel the previous element.
> >
> > Bug 2: virHostdevNetConfigRestore is called for store the hostdevs
> > which may be used by other domain.
> >
> > Signed-off-by: Huanle Han <hanxueluo at gmail.com <mailto:
> hanxueluo at gmail.com>>
> > ---
> >  src/util/virhostdev.c | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> >
> Should this be two separate patches? IOW: Are you fixing two separate
> issues or essentially the same issue? Makes it easier to cherry-pick and
> choose what may need to be backported elsewhere...
>
> If there's a bug associated that would have been "good" to list as well
> as perhaps an example or two...  That is - what XML and command(s)
> sequence(s) prompted you to write the patch(es)...
>
> Also, my attempt to git am -3 your patch fails - perhaps it's your
> mailer configuration.  Did you use git send-email?  or something else...
> I'm seeing html format... So I'm only reviewing based on what I read.
>
> > diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> > index 9678e2b..ecbe5d8 100644
> > --- a/src/util/virhostdev.c
> > +++ b/src/util/virhostdev.c
> > @@ -785,7 +785,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
> > hostdev_mgr,
> >       * them and reset all the devices before re-attach.
> >       * Attach mac and port profile parameters to devices
> >       */
>
> ^^^
> I think the comment here could be updated to make it clear what's being
> done. To just say "Again 4 loops;..." is a bit misleading since the only
> loops I found in the code were in virHostdevPreparePCIDevices where
> there are (currently) 9 such loops!
>
> > -    for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> > +    for (i = 0; i < virPCIDeviceListCount(pcidevs);) {
> >          virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
> >          virPCIDevicePtr activeDev = NULL;
> >
> > @@ -806,6 +806,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
> > hostdev_mgr,
> >          }
> >
> >          virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev);
> > +        i++;
> >      }
>
>
> Not sure I'm seeing why your fix works... Are you claiming that the
> remove from other domain check (e.g. where the code "continue;"'s)
> shouldn't be updating the 'i' value?  All it seems you did was move the
> i++ from the for line to after the second virPCIDeviceListDel in the
> block... Which yes, does reduce the count of devices and I would think
> should be the case that doesn't increment i...
>
> If we have 4 devices [0, 1, 2, 3] and we remove [1], then there are 3
> devices left and i would be 2, but the new array would be [0, 2, 3] and
> thus we don't check [2].
>
> Perhaps this would work/read better as a while loop.
>
> >
> >      /* At this point, any device that had been used by the guest is in
> > @@ -816,9 +817,25 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
> > hostdev_mgr,
> >       * For SRIOV net host devices, unset mac and port profile before
> >       * reset and reattach device
> >       */
> > -    for (i = 0; i < nhostdevs; i++)
> > -        virHostdevNetConfigRestore(hostdevs[i], hostdev_mgr->stateDir,
> > -                                   oldStateDir);
> > +    for (i = 0; i < nhostdevs; i++) {
> > +        virPCIDevicePtr dev;
> > +        virDomainHostdevDefPtr hostdev = hostdevs[i];
> > +        virDomainHostdevSubsysPCIPtr pcisrc =
> > &hostdev->source.subsys.u.pci;
> > +
> > +        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
> > +            hostdev->source.subsys.type !=
> > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI ||
> > +            hostdev->parent.type != VIR_DOMAIN_DEVICE_NET ||
> > +            !hostdev->parent.data.net <http://parent.data.net>)
>                                          ^^^^^^^^^^^^^^^^^^^^^^^^
> This is what makes me believe you sent in html format
>
> It's also directly taken from virHostdevNetConfigRestore... and could be
> considered partially from virHostdevPreparePCIDevices
>
> Suggestion - create a patch that has a new static bool function
> ("isHostdevPCINetDevice") that does the same check passing "hostdev"...
> Then adjust the (currently) two places that make that check in order to
> use the bool function to decide to continue or not. Then this patch
> would use that function here instead.
>
> > +                continue;
> > +
> > +        dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus,
> > +                              pcisrc->addr.slot, pcisrc->addr.function);
> > +        if (virPCIDeviceListFind(pcidevs, dev)) {
> > +            virHostdevNetConfigRestore(hostdev, hostdev_mgr->stateDir,
> > +                                       oldStateDir);
> > +        }
> > +        virPCIDeviceFree(dev);
> > +   }
>
> It's still not quite clear to me from your description why we have to
> search the pcidevs for our device before calling the Restore function.
> Does it matter if it's in or not in the activePCIHostdevs list (the
> comment just before this section).  I think this is one of those cases
> where the code perhaps isn't self documenting and that by adding a few
> more comments along the way we can better describe what we're trying to
> do and why it has to be done.
>
>
> John
> >
> >      for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> >          virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
> > --
> > 2.1.0
> >
> >
> >
> >
> > --
> > libvir-list mailing list
> > libvir-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150326/18360a1b/attachment-0001.htm>


More information about the libvir-list mailing list