[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