Re: [libvirt] [PATCH 3/4] xen: make direct call when there is only one subdriver
On 07/28/2011 02:00 PM, Laine Stump wrote:
On 07/21/2011 05:39 PM, Eric Blake wrote:
> No need to use a for loop if we know there is exactly one client.
>
> VIR_DEBUG("Failed to activate a mandatory sub-driver");
> for (i = 0 ; i< XEN_UNIFIED_NR_DRIVERS ; i++)
> - if (priv->opened[i]) drivers[i]->xenClose(conn);
> + if (priv->opened[i])
> + drivers[i]->xenClose(conn);
I only see whitespace change here. Was there supposed to be more?
This one was whitespace only (our style being that if() and its embedded
statements should be separated lines).
> virMutexDestroy(&priv->lock);
> VIR_FREE(priv);
> conn->privateData = NULL;
> @@ -434,8 +435,8 @@ xenUnifiedClose (virConnectPtr conn)
> virDomainEventCallbackListFree(priv->domainEventCallbacks);
>
> for (i = 0; i< XEN_UNIFIED_NR_DRIVERS; ++i)
> - if (priv->opened[i]&& drivers[i]->xenClose)
> - (void) drivers[i]->xenClose (conn);
> + if (priv->opened[i])
> + drivers[i]->xenClose(conn);
I guess the logic here is that if opened[i] is true, there must have
been a xenOpen(), and if there was a xenOpen() there must be a xenClose()?
Correct. I will also squash in a variant of this hunk from patch 4/4 to
make it more obvious that xenOpen and xenClose callbacks must be
non-NULL (xen-inotify was the best example of using only those two
callbacks):
struct xenUnifiedDriver {
- virDrvOpen xenOpen;
- virDrvClose xenClose;
+ virDrvClose xenClose; /* Only mandatory callback; all others may be
NULL */
> virMutexDestroy(&priv->lock);
> VIR_FREE(conn->privateData);
> @@ -537,14 +538,9 @@ static int
> xenUnifiedNodeGetInfo (virConnectPtr conn, virNodeInfoPtr info)
> {
> GET_PRIVATE(conn);
> - int i;
> -
> - for (i = 0; i< XEN_UNIFIED_NR_DRIVERS; ++i)
> - if (priv->opened[i]&&
> - drivers[i]->xenNodeGetInfo&&
> - drivers[i]->xenNodeGetInfo (conn, info) == 0)
> - return 0;
>
> + if (priv->opened[XEN_UNIFIED_XEND_OFFSET])
> + return xenDaemonNodeGetInfo(conn, info);
> return -1;
For all of these, is it that you've determined by examining all of the
driver info structs that only one driver has a particular callback? Can
we guarantee that will continue to be the case in the future?
The guarantee that there is only one driver using the callback was made
by the formula in the commit comment in patch 4:
for f in $(sed -n 's/.*Drv[^ ]* \([^;]*\);.*/\1/p' src/xen/xen_driver.h)
do
git grep "\(\.\|->\)$f\b" src/xen
done | cat
and looking through the resulting list to see which callback struct
members are used exactly once.
I'll update the commit message accordingly.
The guarantee that this will continue to be the case in the future:
well, after this patch, there are 0 uses of these particular callbacks,
so patch 4 removes the callback member from the struct. If the struct
doesn't have a callback member, then new code can't accidentally add
another callback :)
Conditional ACK, based on satisfactory answers to these questions...
I hope that was satisfactory, because I'm pushing as soon as I also
answer your comments to patch 4 :)
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org