On Tue, 2016-01-26 at 18:59 -0500, John Ferlan wrote:
>
> > diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> > index ab17547..0892dbf 100644
> > --- a/src/util/virhostdev.c
> > +++ b/src/util/virhostdev.c
>
> This comment block starts "We have to use 9 loops here." - that should
> be adjusted too as there are now 7 steps. (Is it any coincidence that
> there are also 7 stages of grief and 7 steps to great health? ;-))
That comment is still there by mistake: the comment
> > + /* Detaching devices from the host involves several steps; each of them
> > + * is described at length below */
was supposed to replace it, the idea being that if we list and describe
briefly the steps in a comment, as we currently do, we have to keep both
the actual comments and the summary in sync with no real gain.
> > + /* Step 1: perform safety checks, eg. ensure the devices are assignable */
> > for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> > virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
> > bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK);
> > @@ -710,7 +709,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr,
> > }
> > }
> >
> > - /* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */
> > + /* Step 2: detach managed devices (i.e. bind to appropriate stub driver).
> > + * detachPCIDevices() will also mark devices as inactive */
>
> s/detachPCIDevices/virHostdevOnlyDetachPCIDevice
>
> Or whatever it ends up being named.
Missed when renaming the function between v1 and v2 :)
> s/will also mark devices as inactive/will place device on inactive list */
Okay.
> > + /* Step 5: reattach managed devices to their host drivers; unmanaged
> > + * devices need no further processing. reattachPCIDevices() will
>
> s/reattachPCIDevices/virHostdevOnlyReattachPCIDevice
>
> (or whatever the final name is)
Same as above :)
> > + * remove devices from the inactive list as they are reattached
> > + * to the host */
> > for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> > virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
> >
> > ignore_value(virHostdevOnlyReattachPCIDevice(hostdev_mgr, dev, true));
> > }
> >
> > + /* At this point, all devices are either marked as inactive (if they
> > + * were unmanaged), or have been reattached to the host driver (if they
> > + * were managed) and are no longer tracked by any of our device lists */
> > +
>
> Ahh, but if you kept the goto cleanup code during what is step 2 here,
> then this wouldn't be necessarily true...
Well, the comment was intended to refer to the situation after Step 5
has been performed, implying that no error that could have caused us to
jump straight to cleanup had occurred.
Maybe that can be made more obvious someway, I'll think about it.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list