Re: [libvirt] [PATCH 20/24] hostdev: Save netdev configuration of actual device
On Wed, 2016-03-16 at 15:05 -0400, John Ferlan wrote: > > > And yes, I'm still curious about virHostdevPCINodeDeviceDetach and how > > > that plays into things. > > > > > What exactly is confusing you about that function? > > It's somewhere that puts devices on the inactiveList via > virPCIDeviceDetach and my eyes/brain kept telling me, don't go there, > you don't want to confuse yourself, stay away, it's a trap. > > I think it was more of how does it play in the whole scheme of things... > Then something written in 21 or 22 perhaps made me think that part of > the process of "self managing" whether the device is attached to the > host or not is something telling libvirt via that function that the > device was detached. It doesn't care about the domain, just the device. Yeah, basically that function is used when the user is willing to take on part of the responsibility himself. Or needs to. If you recall the hostdev series I posted before this one, it was all about making sure that "detach from host" and "reattach to host" do exactly the same thing regardless of whether they're performed explicitly by the user or implicitly as part of working with managed devices. That's the end goal[1]. Cheers. [1] Well, the end goal is really fixing #1372300, but we can't do that in a sane way before we've gotten rid of this code duplication :) -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 20/24] hostdev: Save netdev configuration of actual device
On Wed, 2016-03-16 at 19:50 +0100, Andrea Bolognani wrote: > > Thinking in terms of current code - step4 will take the everything that > > was formerly on the active list and: > > > > 1. Steal the pcidevs entry > > > > 2. Call virHostdevReattachPCIDevice with the stolen lookaside pcidevs > > entry... While it appears to be an actual, it isn't. Of course, I think > > you address that in patch 22, but trying not to get too far ahead. > > > > 2a. If the device is not managed, then add to the inactiveList and > > return (call virPCIDeviceFree if unsuccessfully add). What really > > confuses me here is why we place onto the inactiveList if not managed; > > however, you address that in patch22. Still it could be addressed > > earlier if that is a separate bug... > > I would have to go back to patch 22 to make sure this is really > handled correctly there: I'm pretty confident it is, but I've > been wrong before! Most recently, while writing this series :( > > The incorrect behaviour you've noticed before has actually been > introduced by patch 15. Before that, 'pcidevs' in > virHostdevReAttachPCIDevices() was obtained by copying instances > off the active list, hence virHostdevReattachPCIDevice() would > really be passed (a copy of) an actual; now that's no longer the > case, and the code has become incorrect. > > Will fix as part of the respin. Turns out that fixing this in a separate patch would basically mean reverting the patch that introduced the behaviour in the first place. I don't think it would be worth the effort. I've confirmed that the fix is actually in patch 22, as suspected, so we should be good anyway. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 20/24] hostdev: Save netdev configuration of actual device
[...] > >> And yes, I'm still curious about virHostdevPCINodeDeviceDetach and how >> that plays into things. > > What exactly is confusing you about that function? It's somewhere that puts devices on the inactiveList via virPCIDeviceDetach and my eyes/brain kept telling me, don't go there, you don't want to confuse yourself, stay away, it's a trap. I think it was more of how does it play in the whole scheme of things... Then something written in 21 or 22 perhaps made me think that part of the process of "self managing" whether the device is attached to the host or not is something telling libvirt via that function that the device was detached. It doesn't care about the domain, just the device. > > > So well, it's almost dinner time here, respin's coming tomorrow > I guess :) > I'll go restock the fridge! John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 20/24] hostdev: Save netdev configuration of actual device
On Tue, 2016-03-15 at 14:33 -0400, John Ferlan wrote: > On 03/14/2016 02:00 PM, Andrea Bolognani wrote: > > > > Word of warning, since this is very likely to get extremely > > verbose: I fully expect to slip, here and there, and refer not > > to the *current* situation as of this patch, but to the > > *desired* situation at the end of the series. > > I've rebased to top of tree after your recent pushes... Seems like a > good place to reset focus! I think I need a virtual whiteboard. So, I've already replied to your comments about patch 22 yesterday and patch 21 earlier today. I believe a nice chunk of your concerns have been addressed by my comments in the latter, so I'll mostly go quickly over them. Of course let me know if I've glossed over something that was actually a separate concern. After I'm done with this reply I'll post a shortened series that includes the commits that still haven't been pushed, reordering and fixing them as needed to avoid the problems you noticed in patches 20 and 21. (I might decide to call it a day and go home between these two events, it's getting kind of late already :) > > Such *desired* situation is (not explicitly enough?) laid out > > in patch 19, but I'll expand on it here for clarity: > > > > * the active list and inactive list contain the actual devices > > I understand/agree with the goal for actual devices in each list. One > mental block is usage of virPCIDeviceListAdd for activeList and > virPCIDeviceListAddCopy for inactiveList. This mental block is one of the things patch 22 fixes :) > The second mental block is > whether these are for only managed devices (more on that later). This should have been clarified by my comments to patch 21. > > * each device is contained either in the active list, or in > > the inactive list. It can't be in both > > Sure that's where you're headed, but with the current code this isn't > necessarily true as a device could be on the inactiveList and activeList > during patch 5, but that's the goal of patch 22. Not sure about the reference to patch 5, but yes, this is the *desired* situation, ie. how the code is supposed to behave after the whole series has been applied. If we were already in the desired situation, then we would need no patches ;) > > * virPCIDevice instances in 'pcidevs' are only used for > > looking up actual devices in the bookkeeping lists, contain > > no valuable data and are disposable at any time > > This is where things start getting a bit fuzzy. I agree with the > concept; however, the current implementation has a gotcha. Consider how > VIR_APPEND_ELEMENT works. Initially 'pcidevs' is created as a new List, > then for each 'hostdev' device, a new virPCIDevicePtr is created and > added to the pcidevs list using virPCIDeviceListAdd. It's taken a bit to > have VIR_APPEND_ELEMENT processing sink in for me ;-), but as I read the > Prepare code it makes use of this processing model instead of creating a > separate copy for the activeList. So just to be clear, this is again about using virPCIDeviceListAdd() when adding devices to the active list and virPCIDeviceListAddCopy() when adding them to the inactive list, right? So one of the very issues patch 22 is supposed to clear up :) > > > First off - perhaps something for the previous documentation patch - > > > PrepareDevices is called during the qemuProcessLaunch processing (eg > > > domain startup) for all known host devices. It's also called during > > > qemuDomainAttachHostPCIDevice or libxlDomainAttachHostPCIDevice > > > (hotplug) for the *one* new device to be attached. > > > > > > The purpose of the function is to take the passed hostdev list, move the > > > devices to a managed active host device list for the guest. Devices that > > > do not have the managed attribute are not processed. > > That's not true, though: unmanaged devices *are* processed, even > > if less operations are performed on them. The part about > > detaching them from the host is skipped, because it's supposed > > to have been performed by the user alread, but that's it; > > everything else (resetting them, moving them to the appropriate > > bookkeeping list, storing information about what driver and > > domain is using them) is just the same as managed devices. > > > I think this is where the confusion for me lies. I seem to have also > mistyped a bit - perhaps it's the blurred lines between current and > future perfect (hah!) state. > > Initially an unmanaged device is not placed on the inactiveList (e.g. > step2 in the PreparePCIDevices code), but it is placed on the activeList > (step5). > > Later on, we can place an unmanaged device on the inactiveList either in > virHostdevReattachPCIDevice using the "stolen" 'pcidevs' entry during > virHostdevReAttachPCIDevices or virHostdevPCINodeDeviceDetach where > virPCIDeviceDetach uses a copy of the device. Although patch22 removes > the addition into the inactiveList if !managed (new step5 and deleti
Re: [libvirt] [PATCH 20/24] hostdev: Save netdev configuration of actual device
On 03/14/2016 02:00 PM, Andrea Bolognani wrote: > Word of warning, since this is very likely to get extremely > verbose: I fully expect to slip, here and there, and refer not > to the *current* situation as of this patch, but to the > *desired* situation at the end of the series. I've rebased to top of tree after your recent pushes... Seems like a good place to reset focus! I think I need a virtual whiteboard. > > Such *desired* situation is (not explicitly enough?) laid out > in patch 19, but I'll expand on it here for clarity: > > * the active list and inactive list contain the actual devices I understand/agree with the goal for actual devices in each list. One mental block is usage of virPCIDeviceListAdd for activeList and virPCIDeviceListAddCopy for inactiveList. The second mental block is whether these are for only managed devices (more on that later). > > * each device is contained either in the active list, or in > the inactive list. It can't be in both Sure that's where you're headed, but with the current code this isn't necessarily true as a device could be on the inactiveList and activeList during patch 5, but that's the goal of patch 22. > > * a device that's neither in the active list nor in the > inactive list is assigned to the host OK.. true... hopefully! > > * for each PCI device, only the virPCIDevice instance that is > part of one of the bookkeeping lists matter right. > > * when an operation is to be performed on a PCI device, the > relevant virPCIDevice instance must be looked up in the > appropriate bookkeeping list sure, one would hope! Of course they need to be populated that way... > > * virPCIDevice instances in 'pcidevs' are only used for > looking up actual devices in the bookkeeping lists, contain > no valuable data and are disposable at any time This is where things start getting a bit fuzzy. I agree with the concept; however, the current implementation has a gotcha. Consider how VIR_APPEND_ELEMENT works. Initially 'pcidevs' is created as a new List, then for each 'hostdev' device, a new virPCIDevicePtr is created and added to the pcidevs list using virPCIDeviceListAdd. It's taken a bit to have VIR_APPEND_ELEMENT processing sink in for me ;-), but as I read the Prepare code it makes use of this processing model instead of creating a separate copy for the activeList. > > * exceptions to the above points are to be documented > > Hopefully this makes it easier to see the meaning behind some > of the changes :) > > > On Fri, 2016-03-11 at 14:40 -0500, John Ferlan wrote: >> On 03/10/2016 06:02 PM, John Ferlan wrote: >>> On 03/07/2016 12:24 PM, Andrea Bolognani wrote: >> So I went through this one again (new day, fresh start, partially clear >> mind). > > That often helps :) > So here again, I'm trying to start fresh - go through each patch 1 by 1 so we can "agree" and move to the next one. Save a few electrons, too. Let's try to get to a point where patch22 can be further dissected. >> First off - perhaps something for the previous documentation patch - >> PrepareDevices is called during the qemuProcessLaunch processing (eg >> domain startup) for all known host devices. It's also called during >> qemuDomainAttachHostPCIDevice or libxlDomainAttachHostPCIDevice >> (hotplug) for the *one* new device to be attached. >> >> The purpose of the function is to take the passed hostdev list, move the >> devices to a managed active host device list for the guest. Devices that >> do not have the managed attribute are not processed. > > That's not true, though: unmanaged devices *are* processed, even > if less operations are performed on them. The part about > detaching them from the host is skipped, because it's supposed > to have been performed by the user alread, but that's it; > everything else (resetting them, moving them to the appropriate > bookkeeping list, storing information about what driver and > domain is using them) is just the same as managed devices. > I think this is where the confusion for me lies. I seem to have also mistyped a bit - perhaps it's the blurred lines between current and future perfect (hah!) state. Initially an unmanaged device is not placed on the inactiveList (e.g. step2 in the PreparePCIDevices code), but it is placed on the activeList (step5). Later on, we can place an unmanaged device on the inactiveList either in virHostdevReattachPCIDevice using the "stolen" 'pcidevs' entry during virHostdevReAttachPCIDevices or virHostdevPCINodeDeviceDetach where virPCIDeviceDetach uses a copy of the device. Although patch22 removes the addition into the inactiveList if !managed (new step5 and deletion of lines in ReattachPCIDevice). Still leaves the NodeDeviceDetach path to understand from whence that request came. @@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, >> So after step 6, all the pcidevs are on the activePCIHostdevs list. >> Theoretically at
Re: [libvirt] [PATCH 20/24] hostdev: Save netdev configuration of actual device
Word of warning, since this is very likely to get extremely
verbose: I fully expect to slip, here and there, and refer not
to the *current* situation as of this patch, but to the
*desired* situation at the end of the series.
Such *desired* situation is (not explicitly enough?) laid out
in patch 19, but I'll expand on it here for clarity:
* the active list and inactive list contain the actual devices
* each device is contained either in the active list, or in
the inactive list. It can't be in both
* a device that's neither in the active list nor in the
inactive list is assigned to the host
* for each PCI device, only the virPCIDevice instance that is
part of one of the bookkeeping lists matter
* when an operation is to be performed on a PCI device, the
relevant virPCIDevice instance must be looked up in the
appropriate bookkeeping list
* virPCIDevice instances in 'pcidevs' are only used for
looking up actual devices in the bookkeeping lists, contain
no valuable data and are disposable at any time
* exceptions to the above points are to be documented
Hopefully this makes it easier to see the meaning behind some
of the changes :)
On Fri, 2016-03-11 at 14:40 -0500, John Ferlan wrote:
> On 03/10/2016 06:02 PM, John Ferlan wrote:
> > On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
> So I went through this one again (new day, fresh start, partially clear
> mind).
That often helps :)
> First off - perhaps something for the previous documentation patch -
> PrepareDevices is called during the qemuProcessLaunch processing (eg
> domain startup) for all known host devices. It's also called during
> qemuDomainAttachHostPCIDevice or libxlDomainAttachHostPCIDevice
> (hotplug) for the *one* new device to be attached.
>
> The purpose of the function is to take the passed hostdev list, move the
> devices to a managed active host device list for the guest. Devices that
> do not have the managed attribute are not processed.
That's not true, though: unmanaged devices *are* processed, even
if less operations are performed on them. The part about
detaching them from the host is skipped, because it's supposed
to have been performed by the user alread, but that's it;
everything else (resetting them, moving them to the appropriate
bookkeeping list, storing information about what driver and
domain is using them) is just the same as managed devices.
> > > @@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> > >
> So after step 6, all the pcidevs are on the activePCIHostdevs list.
> Theoretically at least ;-). None are on the inactivePCIHostdevs list.
>
> Why in step 7 do we run through pcidevs, to find the device on the
> activePCIHostdev list in order to set the UsedBy of the actual device on
> the active list. Why not run the 'activePCIHostdev' list here too?
>
> Since we really don't need pcidevs any more - can we delete it now?
> E.G. Step 9 it... Steps 7 & 8 don't really need pcidevs, right? There
> is no error path for either. I'm not asking for a patch - just making
> sure I'm reading things right!
I guess we *could* get rid of it early (even though I haven't
really checked to make sure it would work), but doing so wouldn't
give us any advantage IMHO.
As it is now (well, not as of patch 20, but as of patch 22 which
is basically what this whole series is building towards) 'pcidevs'
is used only for iteration, and iteration is performed only over
'pcidevs'. This is exactly the kind of straightforward knowledge
we need to hold on to if we are ever to get this code in a
manageable state.
Examples of stuff that is easier to reason about if we keep
'pcidevs' around and only ever iterate over it: to obtain an
actual device, we *always* need to look it up in the appropriate
list, no ambiguity; in the cleanup path, we can *always* just
destroy all of 'pcidevs', because no valuable data is stored in
there.
So, even if it might mean performing a couple more lookups here
and there, I think the separation between 'pcidevs' (a list of
instances we can use to look up the actual data) and the actual
bookkeeping lists (where the data we care about is stored) is
crucial and should be enforced.
> > > /* Step 8: Now set the original states for hostdev def */
> > > for (i = 0; i < nhostdevs; i++) {
> > > -virPCIDevicePtr pci;
> > > +virPCIDevicePtr actual;
> > > virDomainHostdevDefPtr hostdev = hostdevs[i];
> > > virDomainHostdevSubsysPCIPtr pcisrc =
> > >&hostdev->source.subsys.u.pci;
> > >
> > > @@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr
> > > mgr,
> > > if (hostdev->source.subsys.type !=
> > >VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> > > continue;
> > >
> > > -pci = virPCIDeviceListFindByIDs(pcidevs,
> > > -pcisrc->addr.domain,
> > > -pcisrc->addr.bus,
> > > -
Re: [libvirt] [PATCH 20/24] hostdev: Save netdev configuration of actual device
On 03/10/2016 06:02 PM, John Ferlan wrote:
>
>
> On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
>> We might be just fine looking up the information in pcidevs, but
>> it wouldn't save us any trouble and it's better to be consistent.
>> ---
>> src/util/virhostdev.c | 41 ++---
>> 1 file changed, 22 insertions(+), 19 deletions(-)
>>
So I went through this one again (new day, fresh start, partially clear
mind).
>> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
>> index a431f0a..03c3445 100644
>> --- a/src/util/virhostdev.c
>> +++ b/src/util/virhostdev.c
First off - perhaps something for the previous documentation patch -
PrepareDevices is called during the qemuProcessLaunch processing (eg
domain startup) for all known host devices. It's also called during
qemuDomainAttachHostPCIDevice or libxlDomainAttachHostPCIDevice
(hotplug) for the *one* new device to be attached.
The purpose of the function is to take the passed hostdev list, move the
devices to a managed active host device list for the guest. Devices that
do not have the managed attribute are not processed.
>> @@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>>
So after step 6, all the pcidevs are on the activePCIHostdevs list.
Theoretically at least ;-). None are on the inactivePCIHostdevs list.
Why in step 7 do we run through pcidevs, to find the device on the
activePCIHostdev list in order to set the UsedBy of the actual device on
the active list. Why not run the 'activePCIHostdev' list here too?
Since we really don't need pcidevs any more - can we delete it now?
E.G. Step 9 it... Steps 7 & 8 don't really need pcidevs, right? There
is no error path for either. I'm not asking for a patch - just making
sure I'm reading things right!
>> /* Step 8: Now set the original states for hostdev def */
>> for (i = 0; i < nhostdevs; i++) {
>> -virPCIDevicePtr pci;
>> +virPCIDevicePtr actual;
>> virDomainHostdevDefPtr hostdev = hostdevs[i];
>> virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
>>
>> @@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>> if (hostdev->source.subsys.type !=
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
>> continue;
>>
>> -pci = virPCIDeviceListFindByIDs(pcidevs,
>> -pcisrc->addr.domain,
>> -pcisrc->addr.bus,
>> -pcisrc->addr.slot,
>> -pcisrc->addr.function);
>> +/* We need to look up the actual device because it's the one
>> + * that contains the information we care about (unbind_from_stub,
>> + * remove_slot, reprobe) */
>
> When a device goes from the inactivePCIHostdevs list to the
> activePCIHostdevs list "at some point in time" in the future - does it
> do a similar save?
I'll answer my own question - because this is the Prepare function and
we don't have to worry about it since everything is on the active list.
>
> That is this change only grabs devices from the active list for the
> save; whereas, prior to this change it would pull from all pcidevs
>
So, I believe this part is correct albeit a bit confusing...
>> +actual = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs,
>> + pcisrc->addr.domain,
>> + pcisrc->addr.bus,
>> + pcisrc->addr.slot,
>> + pcisrc->addr.function);
>>
>> /* Appropriate values for the unbind_from_stub, remove_slot
>> * and reprobe properties of the device were set earlier
>> * by virPCIDeviceDetach() */
>> -if (pci) {
>> +if (actual) {
>> VIR_DEBUG("Saving network configuration of PCI device %s",
>> - virPCIDeviceGetName(pci));
>> + virPCIDeviceGetName(actual));
>> hostdev->origstates.states.pci.unbind_from_stub =
>> -virPCIDeviceGetUnbindFromStub(pci);
>> +virPCIDeviceGetUnbindFromStub(actual);
>> hostdev->origstates.states.pci.remove_slot =
>> -virPCIDeviceGetRemoveSlot(pci);
>> +virPCIDeviceGetRemoveSlot(actual);
>> hostdev->origstates.states.pci.reprobe =
>> -virPCIDeviceGetReprobe(pci);
>> +virPCIDeviceGetReprobe(actual);
>> }
>> }
>>
>> @@ -844,17 +847,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
More documentation thoughts... Because what exists now for the function
doesn't really make sense - it's only describing one variable.
Anyway, the purpose of this function is to take the device(s) passed on
the hostdev list and return them to the host (?if they are managed?).
Re: [libvirt] [PATCH 20/24] hostdev: Save netdev configuration of actual device
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
> We might be just fine looking up the information in pcidevs, but
> it wouldn't save us any trouble and it's better to be consistent.
> ---
> src/util/virhostdev.c | 41 ++---
> 1 file changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index a431f0a..03c3445 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>
> /* Step 8: Now set the original states for hostdev def */
> for (i = 0; i < nhostdevs; i++) {
> -virPCIDevicePtr pci;
> +virPCIDevicePtr actual;
> virDomainHostdevDefPtr hostdev = hostdevs[i];
> virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
>
> @@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> if (hostdev->source.subsys.type !=
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> continue;
>
> -pci = virPCIDeviceListFindByIDs(pcidevs,
> -pcisrc->addr.domain,
> -pcisrc->addr.bus,
> -pcisrc->addr.slot,
> -pcisrc->addr.function);
> +/* We need to look up the actual device because it's the one
> + * that contains the information we care about (unbind_from_stub,
> + * remove_slot, reprobe) */
When a device goes from the inactivePCIHostdevs list to the
activePCIHostdevs list "at some point in time" in the future - does it
do a similar save?
That is this change only grabs devices from the active list for the
save; whereas, prior to this change it would pull from all pcidevs
> +actual = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs,
> + pcisrc->addr.domain,
> + pcisrc->addr.bus,
> + pcisrc->addr.slot,
> + pcisrc->addr.function);
>
> /* Appropriate values for the unbind_from_stub, remove_slot
> * and reprobe properties of the device were set earlier
> * by virPCIDeviceDetach() */
> -if (pci) {
> +if (actual) {
> VIR_DEBUG("Saving network configuration of PCI device %s",
> - virPCIDeviceGetName(pci));
> + virPCIDeviceGetName(actual));
> hostdev->origstates.states.pci.unbind_from_stub =
> -virPCIDeviceGetUnbindFromStub(pci);
> +virPCIDeviceGetUnbindFromStub(actual);
> hostdev->origstates.states.pci.remove_slot =
> -virPCIDeviceGetRemoveSlot(pci);
> +virPCIDeviceGetRemoveSlot(actual);
> hostdev->origstates.states.pci.reprobe =
> -virPCIDeviceGetReprobe(pci);
> +virPCIDeviceGetReprobe(actual);
> }
> }
>
> @@ -844,17 +847,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
>
> if (virHostdevIsPCINetDevice(hostdev)) {
> virDomainHostdevSubsysPCIPtr pcisrc =
> &hostdev->source.subsys.u.pci;
> -virPCIDevicePtr pci;
> +virPCIDevicePtr actual;
>
> -pci = virPCIDeviceListFindByIDs(pcidevs,
> -pcisrc->addr.domain,
> -pcisrc->addr.bus,
> -pcisrc->addr.slot,
> -pcisrc->addr.function);
So the previous loop took care of the activePCIHostdevs, correct? And
this loop takes care of the inactivePCIHostdevs for reattachement? I
think this part is correct - although is it worthy of splitting into
it's own separate patch?
John
Going to stop here for now (we have company).
> +actual = virPCIDeviceListFindByIDs(mgr->inactivePCIHostdevs,
> + pcisrc->addr.domain,
> + pcisrc->addr.bus,
> + pcisrc->addr.slot,
> + pcisrc->addr.function);
>
> -if (pci) {
> +if (actual) {
> VIR_DEBUG("Restoring network configuration of PCI device %s",
> - virPCIDeviceGetName(pci));
> + virPCIDeviceGetName(actual));
> virHostdevNetConfigRestore(hostdev, mgr->stateDir,
> oldStateDir);
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 20/24] hostdev: Save netdev configuration of actual device
We might be just fine looking up the information in pcidevs, but
it wouldn't save us any trouble and it's better to be consistent.
---
src/util/virhostdev.c | 41 ++---
1 file changed, 22 insertions(+), 19 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index a431f0a..03c3445 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
/* Step 8: Now set the original states for hostdev def */
for (i = 0; i < nhostdevs; i++) {
-virPCIDevicePtr pci;
+virPCIDevicePtr actual;
virDomainHostdevDefPtr hostdev = hostdevs[i];
virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
@@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
continue;
-pci = virPCIDeviceListFindByIDs(pcidevs,
-pcisrc->addr.domain,
-pcisrc->addr.bus,
-pcisrc->addr.slot,
-pcisrc->addr.function);
+/* We need to look up the actual device because it's the one
+ * that contains the information we care about (unbind_from_stub,
+ * remove_slot, reprobe) */
+actual = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs,
+ pcisrc->addr.domain,
+ pcisrc->addr.bus,
+ pcisrc->addr.slot,
+ pcisrc->addr.function);
/* Appropriate values for the unbind_from_stub, remove_slot
* and reprobe properties of the device were set earlier
* by virPCIDeviceDetach() */
-if (pci) {
+if (actual) {
VIR_DEBUG("Saving network configuration of PCI device %s",
- virPCIDeviceGetName(pci));
+ virPCIDeviceGetName(actual));
hostdev->origstates.states.pci.unbind_from_stub =
-virPCIDeviceGetUnbindFromStub(pci);
+virPCIDeviceGetUnbindFromStub(actual);
hostdev->origstates.states.pci.remove_slot =
-virPCIDeviceGetRemoveSlot(pci);
+virPCIDeviceGetRemoveSlot(actual);
hostdev->origstates.states.pci.reprobe =
-virPCIDeviceGetReprobe(pci);
+virPCIDeviceGetReprobe(actual);
}
}
@@ -844,17 +847,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
if (virHostdevIsPCINetDevice(hostdev)) {
virDomainHostdevSubsysPCIPtr pcisrc =
&hostdev->source.subsys.u.pci;
-virPCIDevicePtr pci;
+virPCIDevicePtr actual;
-pci = virPCIDeviceListFindByIDs(pcidevs,
-pcisrc->addr.domain,
-pcisrc->addr.bus,
-pcisrc->addr.slot,
-pcisrc->addr.function);
+actual = virPCIDeviceListFindByIDs(mgr->inactivePCIHostdevs,
+ pcisrc->addr.domain,
+ pcisrc->addr.bus,
+ pcisrc->addr.slot,
+ pcisrc->addr.function);
-if (pci) {
+if (actual) {
VIR_DEBUG("Restoring network configuration of PCI device %s",
- virPCIDeviceGetName(pci));
+ virPCIDeviceGetName(actual));
virHostdevNetConfigRestore(hostdev, mgr->stateDir,
oldStateDir);
}
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
