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