[libvirt] [PATCH 3/4] xen: make direct call when there is only one subdriver

Eric Blake eblake at redhat.com
Thu Jul 28 20:34:31 UTC 2011


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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org




More information about the libvir-list mailing list