[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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).

conn->privateData = NULL;
@@ -434,8 +435,8 @@ xenUnifiedClose (virConnectPtr conn)

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 */

@@ -537,14 +538,9 @@ static int
xenUnifiedNodeGetInfo (virConnectPtr conn, virNodeInfoPtr info)
- 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)
  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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]