[libvirt] [PATCH v3 00/12] Make virNodeDeviceObj and virNodeDeviceObjList private
v2: https://www.redhat.com/archives/libvir-list/2017-May/msg00999.html Some patches from v2 were pushed (3, 4, 6, 7, 8, 9, 11, & 12), but a few remained from that series and are the first 5 patches of this series. What changed? -> Reworked the virNodeDeviceObjRemove patch (former patch 2, but new series patch 1). That affected the Test patch (former patch 1, but now patch 2). This patch removes the address of obj logic and moves the onus of the ObjFree to the caller (see patch for reason). -> Patch 3 is the former patch 5, with no essential change -> Patch 4 is the former patch 10, with no essential change -> Former patch 13 and 14, were altered to remove the offending address of pointer logic. The result is patch 5 which just essentially former patch 14 without the address of pointer logic. -> Patches 6-12 for this series are new, but follow along through the logic to make things private. John Ferlan (12): nodedev: Alter virNodeDeviceObjRemove test: Adjust cleanup/error paths for nodedev test APIs nodedev: Use common naming for virnodedeviceobj nodedev: Use consistent names for driver variables nodedev: Introduce virNodeDeviceObjNew nodedev: Introduce virNodeDeviceObjListNew nodedev: Alter node device obj list function names nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs nodedev: Introduce virNodeDeviceGetSCSIHostCaps nodedev: Introduce virNodeDeviceObjListFindSCSIHostByWWNs nodedev: Privatize _virNodeDeviceObj and _virNodeDeviceObjList nodedev: Convert virNodeDeviceObj to use virObjectLockable src/conf/node_device_conf.c | 82 ++++++ src/conf/node_device_conf.h | 20 +- src/conf/virnodedeviceobj.c | 420 ++++++++++++++++++------------ src/conf/virnodedeviceobj.h | 67 ++--- src/libvirt_private.syms | 20 +- src/node_device/node_device_driver.c | 159 +++++------ src/node_device/node_device_hal.c | 47 ++-- src/node_device/node_device_linux_sysfs.c | 77 +----- src/node_device/node_device_udev.c | 52 ++-- src/test/test_driver.c | 136 +++++----- 10 files changed, 570 insertions(+), 510 deletions(-) -- 2.9.4
Rather than passing the object to be removed by reference, pass by value and then let the caller decide whether or not the object should be free'd. This function should just handle the remove of the object from the list for which it was placed during virNodeDeviceObjAssignDef. One caller in node_device_hal would fail to go through the dev_create path since the @dev would have been NULL after returning from the Remove API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 14 ++++++-------- src/conf/virnodedeviceobj.h | 2 +- src/libvirt_private.syms | 1 + src/node_device/node_device_hal.c | 10 ++++++---- src/node_device/node_device_udev.c | 3 ++- src/test/test_driver.c | 8 ++++++-- 6 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index e78f451..fa73de1 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -301,23 +301,21 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr *dev) + virNodeDeviceObjPtr dev) { size_t i; - virNodeDeviceObjUnlock(*dev); + virNodeDeviceObjUnlock(dev); for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(*dev); - if (devs->objs[i] == *dev) { - virNodeDeviceObjUnlock(*dev); - virNodeDeviceObjFree(devs->objs[i]); - *dev = NULL; + virNodeDeviceObjLock(devs->objs[i]); + if (devs->objs[i] == dev) { + virNodeDeviceObjUnlock(devs->objs[i]); VIR_DELETE_ELEMENT(devs->objs, i, devs->count); break; } - virNodeDeviceObjUnlock(*dev); + virNodeDeviceObjUnlock(devs->objs[i]); } } diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 05a9d11..9bc02ee 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -58,7 +58,7 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr *dev); + virNodeDeviceObjPtr dev); int virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 683a232..4a10508 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -959,6 +959,7 @@ virNetworkObjUpdateAssignDef; virNodeDeviceObjAssignDef; virNodeDeviceObjFindByName; virNodeDeviceObjFindBySysfsPath; +virNodeDeviceObjFree; virNodeDeviceObjGetDef; virNodeDeviceObjGetNames; virNodeDeviceObjGetParentHost; diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index f468e42..c354cd3 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -514,7 +514,7 @@ dev_refresh(const char *udi) /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, &dev); + virNodeDeviceObjRemove(&driver->devs, dev); } else { VIR_DEBUG("no device named %s", name); } @@ -543,10 +543,12 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, nodeDeviceLock(); dev = virNodeDeviceObjFindByName(&driver->devs, name); VIR_DEBUG("%s", name); - if (dev) - virNodeDeviceObjRemove(&driver->devs, &dev); - else + if (dev) { + virNodeDeviceObjRemove(&driver->devs, dev); + virNodeDeviceObjFree(dev); + } else { VIR_DEBUG("no device named %s", name); + } nodeDeviceUnlock(); } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 174124a..819e4e7 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1333,7 +1333,8 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); - virNodeDeviceObjRemove(&driver->devs, &dev); + virNodeDeviceObjRemove(&driver->devs, dev); + virNodeDeviceObjFree(dev); if (event) virObjectEventStateQueue(driver->nodeDeviceEventState, event); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e5938f5..e323619 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4530,7 +4530,9 @@ testDestroyVport(testDriverPtr privconn, VIR_NODE_DEVICE_EVENT_DELETED, 0); - virNodeDeviceObjRemove(&privconn->devs, &obj); + virNodeDeviceObjRemove(&privconn->devs, obj); + virNodeDeviceObjFree(obj); + obj = NULL; ret = 0; @@ -5624,7 +5626,9 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) 0); virNodeDeviceObjLock(obj); - virNodeDeviceObjRemove(&driver->devs, &obj); + virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjFree(obj); + obj = NULL; out: if (obj) -- 2.9.4
On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
Rather than passing the object to be removed by reference, pass by value and then let the caller decide whether or not the object should be free'd. This function should just handle the remove of the object from the list for which it was placed during virNodeDeviceObjAssignDef.
One caller in node_device_hal would fail to go through the dev_create path since the @dev would have been NULL after returning from the Remove API.
This is the main motivation for the patch I presume - in which case, I'm wondering why do we actually have to remove the device from the list when handling 'change'/'update' for hal instead of just replacing the ->def with a new instance but it's perfectly fine to do that for udev...I don't see the point in doing what we currently do for hal. [...]
diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index f468e42..c354cd3 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -514,7 +514,7 @@ dev_refresh(const char *udi) /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, &dev); + virNodeDeviceObjRemove(&driver->devs, dev);
I guess now that freeing '@dev' is caller's responsibility, you want to free it on function exit after you checked that you actually want to recreate the device - I already expressed my opinion about this above. ACK with @dev also freed in hal backend. I'd also like to hear your opinion on calling *AssignDef directly from hal's dev_refresh rather than first removing the device from the list completely. Erik
On Thu, Jun 29, 2017 at 02:12:55PM +0200, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
Rather than passing the object to be removed by reference, pass by value and then let the caller decide whether or not the object should be free'd. This function should just handle the remove of the object from the list for which it was placed during virNodeDeviceObjAssignDef.
One caller in node_device_hal would fail to go through the dev_create path since the @dev would have been NULL after returning from the Remove API.
This is the main motivation for the patch I presume - in which case, I'm wondering why do we actually have to remove the device from the list when handling 'change'/'update' for hal instead of just replacing the ->def with a new instance but it's perfectly fine to do that for udev...I don't see the point in doing what we currently do for hal.
It will basically be a historical artifact since few people have touched the HAL driver in years. We only keep it around for compat with FreeBSD IIRC. It is perfectly reasonable to update the code to follow our modern best practice.
diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index f468e42..c354cd3 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -514,7 +514,7 @@ dev_refresh(const char *udi) /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, &dev); + virNodeDeviceObjRemove(&driver->devs, dev);
I guess now that freeing '@dev' is caller's responsibility, you want to free it on function exit after you checked that you actually want to recreate the device - I already expressed my opinion about this above.
ACK with @dev also freed in hal backend. I'd also like to hear your opinion on calling *AssignDef directly from hal's dev_refresh rather than first removing the device from the list completely.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Jun 29, 2017 at 01:24:45PM +0100, Daniel P. Berrange wrote:
On Thu, Jun 29, 2017 at 02:12:55PM +0200, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
Rather than passing the object to be removed by reference, pass by value and then let the caller decide whether or not the object should be free'd. This function should just handle the remove of the object from the list for which it was placed during virNodeDeviceObjAssignDef.
One caller in node_device_hal would fail to go through the dev_create path since the @dev would have been NULL after returning from the Remove API.
This is the main motivation for the patch I presume - in which case, I'm wondering why do we actually have to remove the device from the list when handling 'change'/'update' for hal instead of just replacing the ->def with a new instance but it's perfectly fine to do that for udev...I don't see the point in doing what we currently do for hal.
It will basically be a historical artifact since few people have touched the HAL driver in years. We only keep it around for compat with FreeBSD IIRC. It is perfectly reasonable to update the code to follow our modern best practice.
Thank you for explanation. Erik
On 06/29/2017 08:12 AM, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
Rather than passing the object to be removed by reference, pass by value and then let the caller decide whether or not the object should be free'd. This function should just handle the remove of the object from the list for which it was placed during virNodeDeviceObjAssignDef.
One caller in node_device_hal would fail to go through the dev_create path since the @dev would have been NULL after returning from the Remove API.
This is the main motivation for the patch I presume - in which case, I'm wondering why do we actually have to remove the device from the list when handling 'change'/'update' for hal instead of just replacing the ->def with a new instance but it's perfectly fine to do that for udev...I don't see the point in doing what we currently do for hal.
[...]
The main motivation is that in the previous review pass there was a "dislike" of passing the pointer to a pointer for something else I changed, see: https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html Also the initial pass at altering this function was questioned, see: https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html So I took the approach that this code could/should follow other API's. I used cscope and found other similar functions via "vir.*Obj.*Remove" on the "Find this global definition:" - that led me to 7 functions. Of those Interface, NWFilter, and StoragePool each used this forward linked list in a similar manner - so that's what I changed this model to be. As an aside, nwfilterDefineXML can call virNWFilterObjListRemove, but not clear @obj when done which means the subsequent if (obj) virNWFilterObjUnlock(obj) will fail, but I digress. As you figured out, there's this issue w/ the hal code in dev_refresh() which *never* would have called dev_create() because dev would be NULL *always* if virNodeDeviceObjRemove was called. In hindsight, perhaps I should have solved this by setting a boolean to force calling dev_create(udi) rather than having/forcing each caller to handle calling virNodeDeviceObjFree. Perhaps this gives a flavor for the impetus behind this whole effort - there are perhaps some hidden bugs I'm finding along the way because the existing code (in many places) isn't consistent. I have 8 different models I'm trying to converge - each doing something slightly different. Luckily most issues I've run into have been in error paths or in this case in older and hopefully less used callers.
diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index f468e42..c354cd3 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -514,7 +514,7 @@ dev_refresh(const char *udi) /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, &dev); + virNodeDeviceObjRemove(&driver->devs, dev);
I guess now that freeing '@dev' is caller's responsibility, you want to free it on function exit after you checked that you actually want to recreate the device - I already expressed my opinion about this above.
I'd like to ignore or get rid of the hal code ;-) I think (now) the better option would be to have virNodeDeviceObjRemove make the virNodeDeviceObjFree call as well and have this one caller just know that it ran through this code path in order to force calling dev_create() after releasing the node device lock. Does that seem like a better option?
ACK with @dev also freed in hal backend. I'd also like to hear your opinion on calling *AssignDef directly from hal's dev_refresh rather than first removing the device from the list completely.
Erik
I suppose that's possible, but the comment above the call to virNodeDeviceObjRemove similar scares the sh*t out of me ;-) Tks - John
On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote:
On 06/29/2017 08:12 AM, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
Rather than passing the object to be removed by reference, pass by value and then let the caller decide whether or not the object should be free'd. This function should just handle the remove of the object from the list for which it was placed during virNodeDeviceObjAssignDef.
One caller in node_device_hal would fail to go through the dev_create path since the @dev would have been NULL after returning from the Remove API.
This is the main motivation for the patch I presume - in which case, I'm wondering why do we actually have to remove the device from the list when handling 'change'/'update' for hal instead of just replacing the ->def with a new instance but it's perfectly fine to do that for udev...I don't see the point in doing what we currently do for hal.
[...]
The main motivation is that in the previous review pass there was a "dislike" of passing the pointer to a pointer for something else I changed, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html
Also the initial pass at altering this function was questioned, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html
Well, that comment is true, but the commit message of this patch says that it drops the requirement of passing by reference, thus leaving the responsibility to free the obj to the caller. Now, the way I see it what we should aim at achieving here is reference counted objects, so the vir*ObjFree in the caller would become and *Unref. Therefore IMHO it's not the best approach to introduce another boolean for HAL and leave the vir*ObjFree in the Remove routine, you wouldn't be clearing the object for the caller anyway, so I don't think that is the way to go.
So I took the approach that this code could/should follow other API's. I used cscope and found other similar functions via "vir.*Obj.*Remove" on the "Find this global definition:" - that led me to 7 functions.
I went for basically every module equivalent of this. So the unnecessary lock dance just to compare pointers could be a fairly straightforward follow-up patch across multiple modules if they're consistent.
Of those Interface, NWFilter, and StoragePool each used this forward linked list in a similar manner - so that's what I changed this model to be. As an aside, nwfilterDefineXML can call virNWFilterObjListRemove,
I won't comment on NWFilter as I'm not familiar with it at all.
In hindsight, perhaps I should have solved this by setting a boolean to force calling dev_create(udi) rather than having/forcing each caller to handle calling virNodeDeviceObjFree.
See my comment above, I think that would be a step back in what we're trying to achieve here (comparing it also with other OO languages' practice in this aspect).
/* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, &dev); + virNodeDeviceObjRemove(&driver->devs, dev);
I guess now that freeing '@dev' is caller's responsibility, you want to free it on function exit after you checked that you actually want to recreate the device - I already expressed my opinion about this above.
I'd like to ignore or get rid of the hal code ;-)
Who wouldn't, honestly...
I think (now) the better option would be to have virNodeDeviceObjRemove make the virNodeDeviceObjFree call as well and have this one caller just know that it ran through this code path in order to force calling dev_create() after releasing the node device lock.
Does that seem like a better option?
From my perspective, not really (but I might be wrong of course, wouldn't be
the first nor the last time).
ACK with @dev also freed in hal backend. I'd also like to hear your opinion on calling *AssignDef directly from hal's dev_refresh rather than first removing the device from the list completely.
Erik
I suppose that's possible, but the comment above the call to virNodeDeviceObjRemove similar scares the sh*t out of me ;-)
It just needs a bit of love....and a bunch of cups of coffee ;)...and a platform to test. Erik
On 06/29/2017 10:28 AM, Erik Skultety wrote:
On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote:
On 06/29/2017 08:12 AM, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
Rather than passing the object to be removed by reference, pass by value and then let the caller decide whether or not the object should be free'd. This function should just handle the remove of the object from the list for which it was placed during virNodeDeviceObjAssignDef.
One caller in node_device_hal would fail to go through the dev_create path since the @dev would have been NULL after returning from the Remove API.
This is the main motivation for the patch I presume - in which case, I'm wondering why do we actually have to remove the device from the list when handling 'change'/'update' for hal instead of just replacing the ->def with a new instance but it's perfectly fine to do that for udev...I don't see the point in doing what we currently do for hal.
[...]
The main motivation is that in the previous review pass there was a "dislike" of passing the pointer to a pointer for something else I changed, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html
Also the initial pass at altering this function was questioned, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html
Well, that comment is true, but the commit message of this patch says that it drops the requirement of passing by reference, thus leaving the responsibility to free the obj to the caller. Now, the way I see it what we should aim at achieving here is reference counted objects, so the vir*ObjFree in the caller would become and *Unref. Therefore IMHO it's not the best approach to introduce another boolean for HAL and leave the vir*ObjFree in the Remove routine, you wouldn't be clearing the object for the caller anyway, so I don't think that is the way to go.
I actually think the better course of action is to be more like the other functions. I believe virNodeDeviceObjRemove should do the virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the alloc and list insertion and *Remove is the antecedent doing the list removal and free. I will adjust the commit message and can even add comments prior to the function (if desired; however, it'll eventually be just a wrapper to a more generic object function). Also, light has dawned over marble head and for hal's dev_refresh the logic will then work correctly anyway since &dev wouldn't be set to NULL. The code doesn't reference data in @dev. All that is important is that @dev was found and removed. Still adding a bool rediscover just makes it more obvious for the next poor soul tripping across this code. If you search through history, you'll find that commit '611480747' introduced this hal dev_refresh anomaly in order to fix a problem in testNodeDeviceDestroy that @obj would not be present. Instead of setting obj = NULL; after calls, the usage of &dev was introduced. I'm undoing that and taking the other approach to set the removed @dev = NULL; (all that during a freeze, too!). See the quagmire this convergence creates ;-) Anyway, I've attached a patch that should be able to be git am'd on top of this one. It will cause merge conflicts in the rest of the series... John
So I took the approach that this code could/should follow other API's. I used cscope and found other similar functions via "vir.*Obj.*Remove" on the "Find this global definition:" - that led me to 7 functions.
I went for basically every module equivalent of this. So the unnecessary lock dance just to compare pointers could be a fairly straightforward follow-up patch across multiple modules if they're consistent.
Of those Interface, NWFilter, and StoragePool each used this forward linked list in a similar manner - so that's what I changed this model to be. As an aside, nwfilterDefineXML can call virNWFilterObjListRemove,
I won't comment on NWFilter as I'm not familiar with it at all.
In hindsight, perhaps I should have solved this by setting a boolean to force calling dev_create(udi) rather than having/forcing each caller to handle calling virNodeDeviceObjFree.
See my comment above, I think that would be a step back in what we're trying to achieve here (comparing it also with other OO languages' practice in this aspect).
/* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, &dev); + virNodeDeviceObjRemove(&driver->devs, dev);
I guess now that freeing '@dev' is caller's responsibility, you want to free it on function exit after you checked that you actually want to recreate the device - I already expressed my opinion about this above.
I'd like to ignore or get rid of the hal code ;-)
Who wouldn't, honestly...
I think (now) the better option would be to have virNodeDeviceObjRemove make the virNodeDeviceObjFree call as well and have this one caller just know that it ran through this code path in order to force calling dev_create() after releasing the node device lock.
Does that seem like a better option?
From my perspective, not really (but I might be wrong of course, wouldn't be the first nor the last time).
ACK with @dev also freed in hal backend. I'd also like to hear your opinion on calling *AssignDef directly from hal's dev_refresh rather than first removing the device from the list completely.
Erik
I suppose that's possible, but the comment above the call to virNodeDeviceObjRemove similar scares the sh*t out of me ;-)
It just needs a bit of love....and a bunch of cups of coffee ;)...and a platform to test.
Erik
On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote:
On 06/29/2017 10:28 AM, Erik Skultety wrote:
On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote:
On 06/29/2017 08:12 AM, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
Rather than passing the object to be removed by reference, pass by value and then let the caller decide whether or not the object should be free'd. This function should just handle the remove of the object from the list for which it was placed during virNodeDeviceObjAssignDef.
One caller in node_device_hal would fail to go through the dev_create path since the @dev would have been NULL after returning from the Remove API.
This is the main motivation for the patch I presume - in which case, I'm wondering why do we actually have to remove the device from the list when handling 'change'/'update' for hal instead of just replacing the ->def with a new instance but it's perfectly fine to do that for udev...I don't see the point in doing what we currently do for hal.
[...]
The main motivation is that in the previous review pass there was a "dislike" of passing the pointer to a pointer for something else I changed, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html
Also the initial pass at altering this function was questioned, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html
Well, that comment is true, but the commit message of this patch says that it drops the requirement of passing by reference, thus leaving the responsibility to free the obj to the caller. Now, the way I see it what we should aim at achieving here is reference counted objects, so the vir*ObjFree in the caller would become and *Unref. Therefore IMHO it's not the best approach to introduce another boolean for HAL and leave the vir*ObjFree in the Remove routine, you wouldn't be clearing the object for the caller anyway, so I don't think that is the way to go.
I actually think the better course of action is to be more like the other functions. I believe virNodeDeviceObjRemove should do the virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the alloc and list insertion and *Remove is the antecedent doing the list
From my perspective, if it's the caller who's responsible to free it, they'll know they cannot touch the pointer afterwards (yeah...I assumed there are no bugs) whereas if freeing the object is done as part of ObjRemove, it's the caller who's the one to assume if the object was both freed and cleared or just freed or neither - which I don't like that much, but again, matter of
Well, eventually we'll hopefully end up with reference-counted objects so doing it with having *ObjFree in ObjRemove or in the caller won't matter in the end. perspective, I see your reasoning though. You'll have to replace frees with unrefs anyway, thus you won't make it any easier by this approach, the amount of work in both cases we're discussing here is equal, so I'll be probably fine with that adjustment as well.
removal and free. I will adjust the commit message and can even add comments prior to the function (if desired; however, it'll eventually be just a wrapper to a more generic object function).
Also, light has dawned over marble head and for hal's dev_refresh the logic will then work correctly anyway since &dev wouldn't be set to NULL. The code doesn't reference data in @dev. All that is important is
Relying on a dangling pointer just for the sole purpose of checking 'yep, there used to be something some time ago' would be very very very poor and fragile design. Adding a bool there is still only a workaround of a previous workaround (the commit you referenced), which doesn't necessarily make the 'more nicely packaged' fix a better solution. Per [1] we should call dev_create(udi) right away, dropping the driver lock just before the call - so much more readable. So I don't agree with the boolean part of the patch attached. [1] https://www.redhat.com/archives/libvir-list/2017-June/msg01328.html
On 06/30/2017 04:40 AM, Erik Skultety wrote:
On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote:
On 06/29/2017 10:28 AM, Erik Skultety wrote:
On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote:
On 06/29/2017 08:12 AM, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
Rather than passing the object to be removed by reference, pass by value and then let the caller decide whether or not the object should be free'd. This function should just handle the remove of the object from the list for which it was placed during virNodeDeviceObjAssignDef.
One caller in node_device_hal would fail to go through the dev_create path since the @dev would have been NULL after returning from the Remove API.
This is the main motivation for the patch I presume - in which case, I'm wondering why do we actually have to remove the device from the list when handling 'change'/'update' for hal instead of just replacing the ->def with a new instance but it's perfectly fine to do that for udev...I don't see the point in doing what we currently do for hal.
[...]
The main motivation is that in the previous review pass there was a "dislike" of passing the pointer to a pointer for something else I changed, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html
Also the initial pass at altering this function was questioned, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html
Well, that comment is true, but the commit message of this patch says that it drops the requirement of passing by reference, thus leaving the responsibility to free the obj to the caller. Now, the way I see it what we should aim at achieving here is reference counted objects, so the vir*ObjFree in the caller would become and *Unref. Therefore IMHO it's not the best approach to introduce another boolean for HAL and leave the vir*ObjFree in the Remove routine, you wouldn't be clearing the object for the caller anyway, so I don't think that is the way to go.
I actually think the better course of action is to be more like the other functions. I believe virNodeDeviceObjRemove should do the virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the alloc and list insertion and *Remove is the antecedent doing the list
Well, eventually we'll hopefully end up with reference-counted objects so doing it with having *ObjFree in ObjRemove or in the caller won't matter in the end.
Keeping the Free/Unref as part of the callers responsibility would mean that once we ended up with common object code doing the remove, then each of the nodedev callers would need to remove their extra Free/Unref. Which like you point out below is essentially equal work as long as one remembers that when it comes time ;-).
From my perspective, if it's the caller who's responsible to free it, they'll know they cannot touch the pointer afterwards (yeah...I assumed there are no bugs) whereas if freeing the object is done as part of ObjRemove, it's the caller who's the one to assume if the object was both freed and cleared or just freed or neither - which I don't like that much, but again, matter of perspective, I see your reasoning though. You'll have to replace frees with unrefs anyway, thus you won't make it any easier by this approach, the amount of work in both cases we're discussing here is equal, so I'll be probably fine with that adjustment as well.
removal and free. I will adjust the commit message and can even add comments prior to the function (if desired; however, it'll eventually be just a wrapper to a more generic object function).
Also, light has dawned over marble head and for hal's dev_refresh the logic will then work correctly anyway since &dev wouldn't be set to NULL. The code doesn't reference data in @dev. All that is important is
Relying on a dangling pointer just for the sole purpose of checking 'yep, there used to be something some time ago' would be very very very poor and fragile design. Adding a bool there is still only a workaround of a previous workaround (the commit you referenced), which doesn't necessarily make the 'more nicely packaged' fix a better solution. Per [1] we should call dev_create(udi) right away, dropping the driver lock just before the call - so much more readable. So I don't agree with the boolean part of the patch attached.
Personally I was more in favor of the pass by reference logic because it leaves no question. I'm not as much of a fan of the (sometimes hidden) knowledge that a function "consumes" a callers pointer. We have quite a few of those sprinkled throughout the code. The few code analyzers I've used over time prefer logic that doesn't require some caller to set a pointer they passed to a function to NULL after the function successfully completes. Opinions in this team are different though. Anyway, from your comment to drop the boolean the logic is either: Lock dev = FindByName if (dev) Remove Unlock dev_create else DEBUG Unlock Or adding a return after the dev_create to avoid having a second Unlock in the else. IDC either way, just want to be clear. FWIW: The boolean would be short lived... Once the ObjList is a self locking virLockableObject (patch 13), the boolean would disappear in patch 14 with the dev_create moving inside the "if" part. I already have that in my branch, but I'm sure it wouldn't apply due to merge conflicts as a result of changes made as part of the review process. John
[1] https://www.redhat.com/archives/libvir-list/2017-June/msg01328.html
On Fri, Jun 30, 2017 at 07:14:13AM -0400, John Ferlan wrote:
On 06/30/2017 04:40 AM, Erik Skultety wrote:
On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote:
On 06/29/2017 10:28 AM, Erik Skultety wrote:
On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote:
On 06/29/2017 08:12 AM, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote: > Rather than passing the object to be removed by reference, pass by value > and then let the caller decide whether or not the object should be free'd. > This function should just handle the remove of the object from the list > for which it was placed during virNodeDeviceObjAssignDef. > > One caller in node_device_hal would fail to go through the dev_create path > since the @dev would have been NULL after returning from the Remove API.
This is the main motivation for the patch I presume - in which case, I'm wondering why do we actually have to remove the device from the list when handling 'change'/'update' for hal instead of just replacing the ->def with a new instance but it's perfectly fine to do that for udev...I don't see the point in doing what we currently do for hal.
[...]
The main motivation is that in the previous review pass there was a "dislike" of passing the pointer to a pointer for something else I changed, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html
Also the initial pass at altering this function was questioned, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html
Well, that comment is true, but the commit message of this patch says that it drops the requirement of passing by reference, thus leaving the responsibility to free the obj to the caller. Now, the way I see it what we should aim at achieving here is reference counted objects, so the vir*ObjFree in the caller would become and *Unref. Therefore IMHO it's not the best approach to introduce another boolean for HAL and leave the vir*ObjFree in the Remove routine, you wouldn't be clearing the object for the caller anyway, so I don't think that is the way to go.
I actually think the better course of action is to be more like the other functions. I believe virNodeDeviceObjRemove should do the virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the alloc and list insertion and *Remove is the antecedent doing the list
Well, eventually we'll hopefully end up with reference-counted objects so doing it with having *ObjFree in ObjRemove or in the caller won't matter in the end.
Keeping the Free/Unref as part of the callers responsibility would mean that once we ended up with common object code doing the remove, then each of the nodedev callers would need to remove their extra Free/Unref.
I don't think so. Looking at other examples, what happens in most cases is that caller already gets the ref'd object, calls Remove() which calls Unref(), returns to the caller who then calls EndAPI() which in turn destroys the last reference. Therefore in here, to actually get rid of the object all the callers would need to do essentially the same thing - keep their Unref() in the function block, solely because GetObj() returns an already locked and ref'd object.
Which like you point out below is essentially equal work as long as one remembers that when it comes time ;-).
From my perspective, if it's the caller who's responsible to free it, they'll know they cannot touch the pointer afterwards (yeah...I assumed there are no bugs) whereas if freeing the object is done as part of ObjRemove, it's the caller who's the one to assume if the object was both freed and cleared or just freed or neither - which I don't like that much, but again, matter of perspective, I see your reasoning though. You'll have to replace frees with unrefs anyway, thus you won't make it any easier by this approach, the amount of work in both cases we're discussing here is equal, so I'll be probably fine with that adjustment as well.
removal and free. I will adjust the commit message and can even add comments prior to the function (if desired; however, it'll eventually be just a wrapper to a more generic object function).
Also, light has dawned over marble head and for hal's dev_refresh the logic will then work correctly anyway since &dev wouldn't be set to NULL. The code doesn't reference data in @dev. All that is important is
Relying on a dangling pointer just for the sole purpose of checking 'yep, there used to be something some time ago' would be very very very poor and fragile design. Adding a bool there is still only a workaround of a previous workaround (the commit you referenced), which doesn't necessarily make the 'more nicely packaged' fix a better solution. Per [1] we should call dev_create(udi) right away, dropping the driver lock just before the call - so much more readable. So I don't agree with the boolean part of the patch attached.
Personally I was more in favor of the pass by reference logic because it leaves no question. I'm not as much of a fan of the (sometimes hidden) knowledge that a function "consumes" a callers pointer. We have quite a few of those sprinkled throughout the code. The few code analyzers I've used over time prefer logic that doesn't require some caller to set a pointer they passed to a function to NULL after the function successfully completes. Opinions in this team are different though.
Anyway, from your comment to drop the boolean the logic is either:
Lock dev = FindByName if (dev) Remove
I wanted to get rid of this^ completely....in a separate patch...
Unlock dev_create
and call ^this directly, the remove there doesn't serve a purpose anymore.
else DEBUG Unlock
Or adding a return after the dev_create to avoid having a second Unlock in the else. IDC either way, just want to be clear.
FWIW: The boolean would be short lived... Once the ObjList is a self locking virLockableObject (patch 13), the boolean would disappear in
I know it would - so why introduce it in the first place if we can achieve the same thing in a different way, especially because its removal would be only a couple of patches apart (I know...there are exceptions - this does not qualify as one). Erik
On 06/30/2017 07:41 AM, Erik Skultety wrote:
On Fri, Jun 30, 2017 at 07:14:13AM -0400, John Ferlan wrote:
On 06/30/2017 04:40 AM, Erik Skultety wrote:
On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote:
On 06/29/2017 10:28 AM, Erik Skultety wrote:
On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote:
On 06/29/2017 08:12 AM, Erik Skultety wrote: > On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote: >> Rather than passing the object to be removed by reference, pass by value >> and then let the caller decide whether or not the object should be free'd. >> This function should just handle the remove of the object from the list >> for which it was placed during virNodeDeviceObjAssignDef. >> >> One caller in node_device_hal would fail to go through the dev_create path >> since the @dev would have been NULL after returning from the Remove API. > > This is the main motivation for the patch I presume - in which case, I'm > wondering why do we actually have to remove the device from the list when > handling 'change'/'update' for hal instead of just replacing the ->def with a > new instance but it's perfectly fine to do that for udev...I don't see the > point in doing what we currently do for hal. > > [...]
The main motivation is that in the previous review pass there was a "dislike" of passing the pointer to a pointer for something else I changed, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html
Also the initial pass at altering this function was questioned, see:
https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html
Well, that comment is true, but the commit message of this patch says that it drops the requirement of passing by reference, thus leaving the responsibility to free the obj to the caller. Now, the way I see it what we should aim at achieving here is reference counted objects, so the vir*ObjFree in the caller would become and *Unref. Therefore IMHO it's not the best approach to introduce another boolean for HAL and leave the vir*ObjFree in the Remove routine, you wouldn't be clearing the object for the caller anyway, so I don't think that is the way to go.
I actually think the better course of action is to be more like the other functions. I believe virNodeDeviceObjRemove should do the virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the alloc and list insertion and *Remove is the antecedent doing the list
Well, eventually we'll hopefully end up with reference-counted objects so doing it with having *ObjFree in ObjRemove or in the caller won't matter in the end.
Keeping the Free/Unref as part of the callers responsibility would mean that once we ended up with common object code doing the remove, then each of the nodedev callers would need to remove their extra Free/Unref.
I don't think so. Looking at other examples, what happens in most cases is that caller already gets the ref'd object, calls Remove() which calls Unref(), returns to the caller who then calls EndAPI() which in turn destroys the last reference. Therefore in here, to actually get rid of the object all the callers would need to do essentially the same thing - keep their Unref() in the function block, solely because GetObj() returns an already locked and ref'd object.
Doh... true - of course I have in mind the end goal which is making this whole process awfully difficult. Going step by step hasn't really helped as much as it should have /-(. Adjustments to later patches will be necessary to either Unref or EndAPI. Without getting into specifics of each EndAPI model (domain, network, and secret) - I'm assuming you can agree that each does things a bit differently and gives a sense for the impetus behind this overall effort. I generally sit here looking at code keeping track on my fingers of the RefCnt and whether the lock is held ;-)
Which like you point out below is essentially equal work as long as one remembers that when it comes time ;-).
From my perspective, if it's the caller who's responsible to free it, they'll know they cannot touch the pointer afterwards (yeah...I assumed there are no bugs) whereas if freeing the object is done as part of ObjRemove, it's the caller who's the one to assume if the object was both freed and cleared or just freed or neither - which I don't like that much, but again, matter of perspective, I see your reasoning though. You'll have to replace frees with unrefs anyway, thus you won't make it any easier by this approach, the amount of work in both cases we're discussing here is equal, so I'll be probably fine with that adjustment as well.
removal and free. I will adjust the commit message and can even add comments prior to the function (if desired; however, it'll eventually be just a wrapper to a more generic object function).
Also, light has dawned over marble head and for hal's dev_refresh the logic will then work correctly anyway since &dev wouldn't be set to NULL. The code doesn't reference data in @dev. All that is important is
Relying on a dangling pointer just for the sole purpose of checking 'yep, there used to be something some time ago' would be very very very poor and fragile design. Adding a bool there is still only a workaround of a previous workaround (the commit you referenced), which doesn't necessarily make the 'more nicely packaged' fix a better solution. Per [1] we should call dev_create(udi) right away, dropping the driver lock just before the call - so much more readable. So I don't agree with the boolean part of the patch attached.
Personally I was more in favor of the pass by reference logic because it leaves no question. I'm not as much of a fan of the (sometimes hidden) knowledge that a function "consumes" a callers pointer. We have quite a few of those sprinkled throughout the code. The few code analyzers I've used over time prefer logic that doesn't require some caller to set a pointer they passed to a function to NULL after the function successfully completes. Opinions in this team are different though.
Anyway, from your comment to drop the boolean the logic is either:
Lock dev = FindByName if (dev) Remove
I wanted to get rid of this^ completely....in a separate patch...
Unlock dev_create
and call ^this directly, the remove there doesn't serve a purpose anymore.
I'm not sure I follow... Like noted previously trying not to disturb the logic here - just applying a different band-aid The purpose of the Remove AIUI is to allow the code to rediscover some device because no one wanted to handle the messy situation of some capability being removed or some property being modified. Instead it was deemed a better option to just Remove the device and allow dev_create reinstantiate. Go back to the original implementation, commit id '620d4be7a'... The locks were added as part of commit id 'd48717054c7' and the dev_refresh in commit id '6244c173b' (to fix a deadlock). I think the Remove is necessary.
else DEBUG Unlock
Or adding a return after the dev_create to avoid having a second Unlock in the else. IDC either way, just want to be clear.
FWIW: The boolean would be short lived... Once the ObjList is a self locking virLockableObject (patch 13), the boolean would disappear in
I know it would - so why introduce it in the first place if we can achieve the same thing in a different way, especially because its removal would be only a couple of patches apart (I know...there are exceptions - this does not qualify as one).
Erik
I can remove the bool, IDC, but unless I can understand why you don't think the Remove is necessary - it's either a bool or a move dev_create() inside the if, after an Unlock and a return in the if condition (at least temporarily). John
On Fri, Jun 30, 2017 at 09:47:07AM -0400, John Ferlan wrote:
On 06/30/2017 07:41 AM, Erik Skultety wrote:
On Fri, Jun 30, 2017 at 07:14:13AM -0400, John Ferlan wrote:
On 06/30/2017 04:40 AM, Erik Skultety wrote:
On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote:
On 06/29/2017 10:28 AM, Erik Skultety wrote:
On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote: > > > On 06/29/2017 08:12 AM, Erik Skultety wrote: >> On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote: >>> Rather than passing the object to be removed by reference, pass by value >>> and then let the caller decide whether or not the object should be free'd. >>> This function should just handle the remove of the object from the list >>> for which it was placed during virNodeDeviceObjAssignDef. >>> >>> One caller in node_device_hal would fail to go through the dev_create path >>> since the @dev would have been NULL after returning from the Remove API. >> >> This is the main motivation for the patch I presume - in which case, I'm >> wondering why do we actually have to remove the device from the list when >> handling 'change'/'update' for hal instead of just replacing the ->def with a >> new instance but it's perfectly fine to do that for udev...I don't see the >> point in doing what we currently do for hal. >> >> [...] > > The main motivation is that in the previous review pass there was a > "dislike" of passing the pointer to a pointer for something else I > changed, see: > > https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html > > Also the initial pass at altering this function was questioned, see: > > https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html >
Well, that comment is true, but the commit message of this patch says that it drops the requirement of passing by reference, thus leaving the responsibility to free the obj to the caller. Now, the way I see it what we should aim at achieving here is reference counted objects, so the vir*ObjFree in the caller would become and *Unref. Therefore IMHO it's not the best approach to introduce another boolean for HAL and leave the vir*ObjFree in the Remove routine, you wouldn't be clearing the object for the caller anyway, so I don't think that is the way to go.
I actually think the better course of action is to be more like the other functions. I believe virNodeDeviceObjRemove should do the virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the alloc and list insertion and *Remove is the antecedent doing the list
Well, eventually we'll hopefully end up with reference-counted objects so doing it with having *ObjFree in ObjRemove or in the caller won't matter in the end.
Keeping the Free/Unref as part of the callers responsibility would mean that once we ended up with common object code doing the remove, then each of the nodedev callers would need to remove their extra Free/Unref.
I don't think so. Looking at other examples, what happens in most cases is that caller already gets the ref'd object, calls Remove() which calls Unref(), returns to the caller who then calls EndAPI() which in turn destroys the last reference. Therefore in here, to actually get rid of the object all the callers would need to do essentially the same thing - keep their Unref() in the function block, solely because GetObj() returns an already locked and ref'd object.
Doh... true - of course I have in mind the end goal which is making this whole process awfully difficult. Going step by step hasn't really helped as much as it should have /-(. Adjustments to later patches will be necessary to either Unref or EndAPI.
I'm sorry, I don't follow. What changes to later patches are we talking about exactly? I agree with the original approach you took because IMHO it is the most transparent in terms of gradual changes - replacing frees with unrefs or EndAPIs for that matter, and it also keeps things consistent.
Without getting into specifics of each EndAPI model (domain, network, and secret) - I'm assuming you can agree that each does things a bit differently and gives a sense for the impetus behind this overall effort. I generally sit here looking at code keeping track on my fingers of the RefCnt and whether the lock is held ;-)
Which like you point out below is essentially equal work as long as one remembers that when it comes time ;-).
From my perspective, if it's the caller who's responsible to free it, they'll know they cannot touch the pointer afterwards (yeah...I assumed there are no bugs) whereas if freeing the object is done as part of ObjRemove, it's the caller who's the one to assume if the object was both freed and cleared or just freed or neither - which I don't like that much, but again, matter of perspective, I see your reasoning though. You'll have to replace frees with unrefs anyway, thus you won't make it any easier by this approach, the amount of work in both cases we're discussing here is equal, so I'll be probably fine with that adjustment as well.
removal and free. I will adjust the commit message and can even add comments prior to the function (if desired; however, it'll eventually be just a wrapper to a more generic object function).
Also, light has dawned over marble head and for hal's dev_refresh the logic will then work correctly anyway since &dev wouldn't be set to NULL. The code doesn't reference data in @dev. All that is important is
Relying on a dangling pointer just for the sole purpose of checking 'yep, there used to be something some time ago' would be very very very poor and fragile design. Adding a bool there is still only a workaround of a previous workaround (the commit you referenced), which doesn't necessarily make the 'more nicely packaged' fix a better solution. Per [1] we should call dev_create(udi) right away, dropping the driver lock just before the call - so much more readable. So I don't agree with the boolean part of the patch attached.
Personally I was more in favor of the pass by reference logic because it leaves no question. I'm not as much of a fan of the (sometimes hidden) knowledge that a function "consumes" a callers pointer. We have quite a few of those sprinkled throughout the code. The few code analyzers I've used over time prefer logic that doesn't require some caller to set a pointer they passed to a function to NULL after the function successfully completes. Opinions in this team are different though.
Anyway, from your comment to drop the boolean the logic is either:
Lock dev = FindByName if (dev) Remove
I wanted to get rid of this^ completely....in a separate patch...
Unlock dev_create
and call ^this directly, the remove there doesn't serve a purpose anymore.
I'm not sure I follow... Like noted previously trying not to disturb the logic here - just applying a different band-aid
The purpose of the Remove AIUI is to allow the code to rediscover some device because no one wanted to handle the messy situation of some capability being removed or some property being modified. Instead it was deemed a better option to just Remove the device and allow dev_create reinstantiate.
It's been a long time since 2008 and there wasn't a exhausting description why it was a problem back in 2008 - locks maybe?. Commits d48717054c7 and 6244c173b don't really relate to what I'm suggesting, do they? You wouldn't mess around with locks, so you wouldn't regress with deadlock. I'm still trying to figure out what I'm missing here, since you're holding all the necessary locks anyway so what could possibly be the benefit of calling remove on the device prior to creating-refreshing it next. Erik
Go back to the original implementation, commit id '620d4be7a'... The locks were added as part of commit id 'd48717054c7' and the dev_refresh in commit id '6244c173b' (to fix a deadlock).
I think the Remove is necessary.
else DEBUG Unlock
Or adding a return after the dev_create to avoid having a second Unlock in the else. IDC either way, just want to be clear.
FWIW: The boolean would be short lived... Once the ObjList is a self locking virLockableObject (patch 13), the boolean would disappear in
I know it would - so why introduce it in the first place if we can achieve the same thing in a different way, especially because its removal would be only a couple of patches apart (I know...there are exceptions - this does not qualify as one).
Erik
I can remove the bool, IDC, but unless I can understand why you don't think the Remove is necessary - it's either a bool or a move dev_create() inside the if, after an Unlock and a return in the if condition (at least temporarily).
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[...]
Doh... true - of course I have in mind the end goal which is making this whole process awfully difficult. Going step by step hasn't really helped as much as it should have /-(. Adjustments to later patches will be necessary to either Unref or EndAPI.
I'm sorry, I don't follow. What changes to later patches are we talking about exactly? I agree with the original approach you took because IMHO it is the most transparent in terms of gradual changes - replacing frees with unrefs or EndAPIs for that matter, and it also keeps things consistent.
Pay no attention to the old man talking to himself ;-) I've gone back to the original 1/12 and just added a virNodeDeviceObjFree within the if (dev) prior to dev_create(udi). Since it doesn't matter if the node device is unlocked it's a safe place to put it. [...]
I'm not sure I follow... Like noted previously trying not to disturb the logic here - just applying a different band-aid
The purpose of the Remove AIUI is to allow the code to rediscover some device because no one wanted to handle the messy situation of some capability being removed or some property being modified. Instead it was deemed a better option to just Remove the device and allow dev_create reinstantiate.
It's been a long time since 2008 and there wasn't a exhausting description why it was a problem back in 2008 - locks maybe?. Commits d48717054c7 and 6244c173b don't really relate to what I'm suggesting, do they? You wouldn't mess around with locks, so you wouldn't regress with deadlock. I'm still trying to figure out what I'm missing here, since you're holding all the necessary locks anyway so what could possibly be the benefit of calling remove on the device prior to creating-refreshing it next.
Erik
I was trying to figure out why you were advocating removing the Find and Remove and just call dev_create directly. AIUI, the code is removing the device and allowing dev_create recreate it because it was easier than deal with def->caps replacement. Sure maybe that's easier today, but it's been way too long for me to recall much about hal other than something to do with 2001: A Space Odyssey ("I'm sorry Dave, I'm afraid I can't do that")... John v4 will be posted soon
- Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName an @obj, just return directly. This then allows the cleanup: label code to not have to check "if (obj)" before calling virNodeDeviceObjUnlock. This also simplifies some exit logic... - In testNodeDeviceObjFindByName use an error: label to handle the failure and don't do the ncaps++ within the VIR_STRDUP() source target index. Only increment ncaps after success. Easier on eyes at error label too. - In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 75 +++++++++++++++++++------------------------------- 1 file changed, 29 insertions(+), 46 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e323619..e91dfa3 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4509,7 +4509,6 @@ testDestroyVport(testDriverPtr privconn, const char *wwnn ATTRIBUTE_UNUSED, const char *wwpn ATTRIBUTE_UNUSED) { - int ret = -1; virNodeDeviceObjPtr obj = NULL; virObjectEventPtr event = NULL; @@ -4523,7 +4522,7 @@ testDestroyVport(testDriverPtr privconn, if (!(obj = virNodeDeviceObjFindByName(&privconn->devs, "scsi_host12"))) { virReportError(VIR_ERR_NO_NODE_DEVICE, "%s", _("no node device with matching name 'scsi_host12'")); - goto cleanup; + return -1; } event = virNodeDeviceEventLifecycleNew("scsi_host12", @@ -4534,13 +4533,8 @@ testDestroyVport(testDriverPtr privconn, virNodeDeviceObjFree(obj); obj = NULL; - ret = 0; - - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); testObjectEventQueue(privconn, event); - return ret; + return 0; } @@ -5328,7 +5322,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name) virNodeDevicePtr ret = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, name))) - goto cleanup; + return NULL; def = virNodeDeviceObjGetDef(obj); if ((ret = virGetNodeDevice(conn, name))) { @@ -5338,9 +5332,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name) } } - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5355,13 +5347,11 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev, virCheckFlags(0, NULL); if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return NULL; ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj)); - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5374,7 +5364,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) char *ret = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return NULL; def = virNodeDeviceObjGetDef(obj); if (def->parent) { @@ -5384,9 +5374,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) "%s", _("no parent for this device")); } - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5399,20 +5387,16 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev) virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; def = virNodeDeviceObjGetDef(obj); for (caps = def->caps; caps; caps = caps->next) ++ncaps; - ret = ncaps; - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); - return ret; + virNodeDeviceObjUnlock(obj); + return ncaps; } @@ -5424,27 +5408,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; def = virNodeDeviceObjGetDef(obj); for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) { - if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0) - goto cleanup; + if (VIR_STRDUP(names[ncaps], + virNodeDevCapTypeToString(caps->data.type)) < 0) + goto error; + ncaps++; } - ret = ncaps; - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); - if (ret == -1) { - --ncaps; - while (--ncaps >= 0) - VIR_FREE(names[ncaps]); - } - return ret; + virNodeDeviceObjUnlock(obj); + return ncaps; + + error: + while (--ncaps >= 0) + VIR_FREE(names[ncaps]); + virNodeDeviceObjUnlock(obj); + return -1; } @@ -5598,14 +5581,14 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virObjectEventPtr event = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto out; + return -1; def = virNodeDeviceObjGetDef(obj); if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) - goto out; + goto cleanup; if (VIR_STRDUP(parent_name, def->parent) < 0) - goto out; + goto cleanup; /* virNodeDeviceGetParentHost will cause the device object's lock to be * taken, so we have to dup the parent's name and drop the lock @@ -5618,7 +5601,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) if (virNodeDeviceObjGetParentHost(&driver->devs, def, EXISTING_DEVICE) < 0) { obj = NULL; - goto out; + goto cleanup; } event = virNodeDeviceEventLifecycleNew(dev->name, @@ -5630,7 +5613,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virNodeDeviceObjFree(obj); obj = NULL; - out: + cleanup: if (obj) virNodeDeviceObjUnlock(obj); testObjectEventQueue(driver, event); -- 2.9.4
On Sat, Jun 03, 2017 at 09:11:52AM -0400, John Ferlan wrote:
- Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName an @obj, just return directly. This then allows the cleanup: label code to not have to check "if (obj)" before calling virNodeDeviceObjUnlock. This also simplifies some exit logic...
- In testNodeDeviceObjFindByName use an error: label to handle the failure and don't do the ncaps++ within the VIR_STRDUP() source target index. Only increment ncaps after success. Easier on eyes at error label too.
- In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto
Signed-off-by: John Ferlan <jferlan@redhat.com>
[..]
event = virNodeDeviceEventLifecycleNew("scsi_host12", @@ -4534,13 +4533,8 @@ testDestroyVport(testDriverPtr privconn, virNodeDeviceObjFree(obj); obj = NULL;
You can drop ^this then since you don't need to clear it anymore. Just a minor nit, while I was checking where testDestroyVport gets called from - testStoragePoolDestroy, there's a spot that could be fixed the same way. Would you mind squashing this bit in as well? diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6422ba8fb..a397364c7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4547,7 +4547,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool) virObjectEventPtr event = NULL; if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name))) - goto cleanup; + return -1; if (!virStoragePoolObjIsActive(privpool)) { virReportError(VIR_ERR_OPERATION_INVALID, [...]
@@ -5424,27 +5408,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; def = virNodeDeviceObjGetDef(obj);
for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) { - if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0) - goto cleanup; + if (VIR_STRDUP(names[ncaps], + virNodeDevCapTypeToString(caps->data.type)) < 0) + goto error; + ncaps++;
How about placing the increment to the iteration_expression segment of the loop, aka at the end where the usual increment happens before condition evaluation. Wondering whether there's a general technical term for the syntax part of the loop between the parentheses.
} - ret = ncaps;
- cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); - if (ret == -1) { - --ncaps; - while (--ncaps >= 0) - VIR_FREE(names[ncaps]);
Hmm, this was indeed an interesting bit of code.
- } - return ret; + virNodeDeviceObjUnlock(obj); + return ncaps; + + error: + while (--ncaps >= 0) + VIR_FREE(names[ncaps]); + virNodeDeviceObjUnlock(obj); + return -1; }
ACK with adjustments. Erik
On 06/29/2017 08:57 AM, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:52AM -0400, John Ferlan wrote:
- Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName an @obj, just return directly. This then allows the cleanup: label code to not have to check "if (obj)" before calling virNodeDeviceObjUnlock. This also simplifies some exit logic...
- In testNodeDeviceObjFindByName use an error: label to handle the failure and don't do the ncaps++ within the VIR_STRDUP() source target index. Only increment ncaps after success. Easier on eyes at error label too.
- In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto
Signed-off-by: John Ferlan <jferlan@redhat.com>
[..]
event = virNodeDeviceEventLifecycleNew("scsi_host12", @@ -4534,13 +4533,8 @@ testDestroyVport(testDriverPtr privconn, virNodeDeviceObjFree(obj); obj = NULL;
You can drop ^this then since you don't need to clear it anymore.
True, I'll drop it especially since the reason it was there to avoid the virNodeDeviceObjUnlock call is no longer valid.
Just a minor nit, while I was checking where testDestroyVport gets called from - testStoragePoolDestroy, there's a spot that could be fixed the same way. Would you mind squashing this bit in as well?
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6422ba8fb..a397364c7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4547,7 +4547,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool) virObjectEventPtr event = NULL;
if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name))) - goto cleanup; + return -1;
I don't mind changing this as well - although I probably have that in another patch somewhere dealing with storage tests. Trying to keep nodedev with nodedev and storage with storage.
if (!virStoragePoolObjIsActive(privpool)) { virReportError(VIR_ERR_OPERATION_INVALID,
[...]
@@ -5424,27 +5408,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; def = virNodeDeviceObjGetDef(obj);
for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) { - if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0) - goto cleanup; + if (VIR_STRDUP(names[ncaps], + virNodeDevCapTypeToString(caps->data.type)) < 0) + goto error; + ncaps++;
How about placing the increment to the iteration_expression segment of the loop, aka at the end where the usual increment happens before condition evaluation. Wondering whether there's a general technical term for the syntax part of the loop between the parentheses.
Do you mean : (caps = def->caps; caps && ncaps < maxnames; caps = caps->next, ncaps++) ?? If so, yuck. Since we're iterating on caps, I think what should be incremented is caps and not ncaps as well. Putting it after the VIR_STRDUP() just makes it clearer that we successfully added something. I'd prefer to keep as is. Tks - John
} - ret = ncaps;
- cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); - if (ret == -1) { - --ncaps; - while (--ncaps >= 0) - VIR_FREE(names[ncaps]);
Hmm, this was indeed an interesting bit of code.
- } - return ret; + virNodeDeviceObjUnlock(obj); + return ncaps; + + error: + while (--ncaps >= 0) + VIR_FREE(names[ncaps]); + virNodeDeviceObjUnlock(obj); + return -1; }
ACK with adjustments.
Erik
Just a minor nit, while I was checking where testDestroyVport gets called from - testStoragePoolDestroy, there's a spot that could be fixed the same way. Would you mind squashing this bit in as well?
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6422ba8fb..a397364c7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4547,7 +4547,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool) virObjectEventPtr event = NULL;
if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name))) - goto cleanup; + return -1;
I don't mind changing this as well - although I probably have that in another patch somewhere dealing with storage tests. Trying to keep nodedev with nodedev and storage with storage.
I figured, but this is where these two world clash, you know, calling nodedev stuff from storage... I don't care if you fix in this one or in some other storage related patch, just so we don't forget about that.
if (!virStoragePoolObjIsActive(privpool)) { virReportError(VIR_ERR_OPERATION_INVALID,
[...]
@@ -5424,27 +5408,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; def = virNodeDeviceObjGetDef(obj);
for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) { - if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0) - goto cleanup; + if (VIR_STRDUP(names[ncaps], + virNodeDevCapTypeToString(caps->data.type)) < 0) + goto error; + ncaps++;
How about placing the increment to the iteration_expression segment of the loop, aka at the end where the usual increment happens before condition evaluation. Wondering whether there's a general technical term for the syntax part of the loop between the parentheses.
Do you mean :
(caps = def->caps; caps && ncaps < maxnames; caps = caps->next, ncaps++)
??
If so, yuck. Since we're iterating on caps, I think what should be incremented is caps and not ncaps as well. Putting it after the
Well, if we only cared about the @caps in the evaluation condition, maybe, but we also check @ncaps and I think it's pretty clear that ncaps are only incremented when VIR_STRDUP passed, otherwise we would have dropped from the loop in the first place. I don't intend to argue about this, as it's not a show stopper, so I don't really care that much about that, I'm fine either way. Erik
A virNodeDeviceObjPtr is an @obj A virNodeDeviceObjListPtr is an @devs Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 124 ++++++++++++++++++++++---------------------- 1 file changed, 62 insertions(+), 62 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index fa73de1..3aff5ca 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -41,10 +41,10 @@ virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj) static int -virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, +virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, const char *cap) { - virNodeDevCapsDefPtr caps = dev->def->caps; + virNodeDevCapsDefPtr caps = obj->def->caps; const char *fc_host_cap = virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_FC_HOST); const char *vports_cap = @@ -105,9 +105,9 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, * Pointer to the caps or NULL if not found */ static virNodeDevCapsDefPtr -virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev) +virNodeDeviceFindFCCapDef(const virNodeDeviceObj *obj) { - virNodeDevCapsDefPtr caps = dev->def->caps; + virNodeDevCapsDefPtr caps = obj->def->caps; while (caps) { if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST && @@ -129,9 +129,9 @@ virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev) * Pointer to the caps or NULL if not found */ static virNodeDevCapsDefPtr -virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *dev) +virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj) { - virNodeDevCapsDefPtr caps = dev->def->caps; + virNodeDevCapsDefPtr caps = obj->def->caps; while (caps) { if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST && @@ -240,16 +240,16 @@ virNodeDeviceFindByCap(virNodeDeviceObjListPtr devs, void -virNodeDeviceObjFree(virNodeDeviceObjPtr dev) +virNodeDeviceObjFree(virNodeDeviceObjPtr obj) { - if (!dev) + if (!obj) return; - virNodeDeviceDefFree(dev->def); + virNodeDeviceDefFree(obj->def); - virMutexDestroy(&dev->lock); + virMutexDestroy(&obj->lock); - VIR_FREE(dev); + VIR_FREE(obj); } @@ -268,48 +268,48 @@ virNodeDeviceObjPtr virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def) { - virNodeDeviceObjPtr device; + virNodeDeviceObjPtr obj; - if ((device = virNodeDeviceObjFindByName(devs, def->name))) { - virNodeDeviceDefFree(device->def); - device->def = def; - return device; + if ((obj = virNodeDeviceObjFindByName(devs, def->name))) { + virNodeDeviceDefFree(obj->def); + obj->def = def; + return obj; } - if (VIR_ALLOC(device) < 0) + if (VIR_ALLOC(obj) < 0) return NULL; - if (virMutexInit(&device->lock) < 0) { + if (virMutexInit(&obj->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); - VIR_FREE(device); + VIR_FREE(obj); return NULL; } - virNodeDeviceObjLock(device); + virNodeDeviceObjLock(obj); - if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, device) < 0) { - virNodeDeviceObjUnlock(device); - virNodeDeviceObjFree(device); + if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) { + virNodeDeviceObjUnlock(obj); + virNodeDeviceObjFree(obj); return NULL; } - device->def = def; + obj->def = def; - return device; + return obj; } void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr dev) + virNodeDeviceObjPtr obj) { size_t i; - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); for (i = 0; i < devs->count; i++) { virNodeDeviceObjLock(devs->objs[i]); - if (devs->objs[i] == dev) { + if (devs->objs[i] == obj) { virNodeDeviceObjUnlock(devs->objs[i]); VIR_DELETE_ELEMENT(devs->objs, i, devs->count); @@ -333,15 +333,15 @@ virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, * parent_host value on success (>= 0), -1 otherwise. */ static int -virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr parent) +virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr obj) { - virNodeDevCapsDefPtr cap = virNodeDeviceFindVPORTCapDef(parent); + virNodeDevCapsDefPtr cap = virNodeDeviceFindVPORTCapDef(obj); if (!cap) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Parent device %s is not capable " "of vport operations"), - parent->def->name); + obj->def->name); return -1; } @@ -354,19 +354,19 @@ virNodeDeviceGetParentHostByParent(virNodeDeviceObjListPtr devs, const char *dev_name, const char *parent_name) { - virNodeDeviceObjPtr parent = NULL; + virNodeDeviceObjPtr obj = NULL; int ret; - if (!(parent = virNodeDeviceObjFindByName(devs, parent_name))) { + if (!(obj = virNodeDeviceObjFindByName(devs, parent_name))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); return -1; } - ret = virNodeDeviceFindFCParentHost(parent); + ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(parent); + virNodeDeviceObjUnlock(obj); return ret; } @@ -378,19 +378,19 @@ virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs, const char *parent_wwnn, const char *parent_wwpn) { - virNodeDeviceObjPtr parent = NULL; + virNodeDeviceObjPtr obj = NULL; int ret; - if (!(parent = virNodeDeviceFindByWWNs(devs, parent_wwnn, parent_wwpn))) { + if (!(obj = virNodeDeviceFindByWWNs(devs, parent_wwnn, parent_wwpn))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); return -1; } - ret = virNodeDeviceFindFCParentHost(parent); + ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(parent); + virNodeDeviceObjUnlock(obj); return ret; } @@ -401,19 +401,19 @@ virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, const char *dev_name, const char *parent_fabric_wwn) { - virNodeDeviceObjPtr parent = NULL; + virNodeDeviceObjPtr obj = NULL; int ret; - if (!(parent = virNodeDeviceFindByFabricWWN(devs, parent_fabric_wwn))) { + if (!(obj = virNodeDeviceFindByFabricWWN(devs, parent_fabric_wwn))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); return -1; } - ret = virNodeDeviceFindFCParentHost(parent); + ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(parent); + virNodeDeviceObjUnlock(obj); return ret; } @@ -422,19 +422,19 @@ virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, static int virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs) { - virNodeDeviceObjPtr parent = NULL; + virNodeDeviceObjPtr obj = NULL; const char *cap = virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_VPORTS); int ret; - if (!(parent = virNodeDeviceFindByCap(devs, cap))) { + if (!(obj = virNodeDeviceFindByCap(devs, cap))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find any vport capable device")); return -1; } - ret = virNodeDeviceFindFCParentHost(parent); + ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(parent); + virNodeDeviceObjUnlock(obj); return ret; } @@ -482,12 +482,12 @@ virNodeDeviceObjUnlock(virNodeDeviceObjPtr obj) static bool -virNodeDeviceCapMatch(virNodeDeviceObjPtr devobj, +virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, int type) { virNodeDevCapsDefPtr cap = NULL; - for (cap = devobj->def->caps; cap; cap = cap->next) { + for (cap = obj->def->caps; cap; cap = cap->next) { if (type == cap->data.type) return true; @@ -588,9 +588,9 @@ virNodeDeviceObjGetNames(virNodeDeviceObjListPtr devs, #define MATCH(FLAG) ((flags & (VIR_CONNECT_LIST_NODE_DEVICES_CAP_ ## FLAG)) && \ - virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_ ## FLAG)) + virNodeDeviceCapMatch(obj, VIR_NODE_DEV_CAP_ ## FLAG)) static bool -virNodeDeviceMatch(virNodeDeviceObjPtr devobj, +virNodeDeviceMatch(virNodeDeviceObjPtr obj, unsigned int flags) { /* filter by cap type */ @@ -621,7 +621,7 @@ virNodeDeviceMatch(virNodeDeviceObjPtr devobj, int virNodeDeviceObjListExport(virConnectPtr conn, - virNodeDeviceObjListPtr devobjs, + virNodeDeviceObjListPtr devs, virNodeDevicePtr **devices, virNodeDeviceObjListFilter filter, unsigned int flags) @@ -632,26 +632,26 @@ virNodeDeviceObjListExport(virConnectPtr conn, int ret = -1; size_t i; - if (devices && VIR_ALLOC_N(tmp_devices, devobjs->count + 1) < 0) + if (devices && VIR_ALLOC_N(tmp_devices, devs->count + 1) < 0) goto cleanup; - for (i = 0; i < devobjs->count; i++) { - virNodeDeviceObjPtr devobj = devobjs->objs[i]; - virNodeDeviceObjLock(devobj); - if ((!filter || filter(conn, devobj->def)) && - virNodeDeviceMatch(devobj, flags)) { + for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDeviceObjLock(obj); + if ((!filter || filter(conn, obj->def)) && + virNodeDeviceMatch(obj, flags)) { if (devices) { - if (!(device = virGetNodeDevice(conn, devobj->def->name)) || - VIR_STRDUP(device->parent, devobj->def->parent) < 0) { + if (!(device = virGetNodeDevice(conn, obj->def->name)) || + VIR_STRDUP(device->parent, obj->def->parent) < 0) { virObjectUnref(device); - virNodeDeviceObjUnlock(devobj); + virNodeDeviceObjUnlock(obj); goto cleanup; } tmp_devices[ndevices] = device; } ndevices++; } - virNodeDeviceObjUnlock(devobj); + virNodeDeviceObjUnlock(obj); } if (tmp_devices) { -- 2.9.4
On Sat, Jun 03, 2017 at 09:11:53AM -0400, John Ferlan wrote:
A virNodeDeviceObjPtr is an @obj
A virNodeDeviceObjListPtr is an @devs
There was some argument that this messes up with history in terms of nt introducing any functional change, thus making it harder to go through the commits and search for something relevant - true, can be painful as hell, but truth to be told, long are the times gone when that was actually the case, given the number of pure code movements etc. we had. And to me, consistency which enhances readability even slightly matters to me a lot. [..]
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 124 ++++++++++++++++++++++---------------------- 1 file changed, 62 insertions(+), 62 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index fa73de1..3aff5ca 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -41,10 +41,10 @@ virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj)
static int -virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, +virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, const char *cap) { - virNodeDevCapsDefPtr caps = dev->def->caps; + virNodeDevCapsDefPtr caps = obj->def->caps; const char *fc_host_cap = virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_FC_HOST); const char *vports_cap = @@ -105,9 +105,9 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, * Pointer to the caps or NULL if not found */ static virNodeDevCapsDefPtr -virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev) +virNodeDeviceFindFCCapDef(const virNodeDeviceObj *obj)
You should update the name of the var in the function commentary as well.
{ - virNodeDevCapsDefPtr caps = dev->def->caps; + virNodeDevCapsDefPtr caps = obj->def->caps;
while (caps) { if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST && @@ -129,9 +129,9 @@ virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev) * Pointer to the caps or NULL if not found */ static virNodeDevCapsDefPtr -virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *dev) +virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj)
Here too. [..]
@@ -333,15 +333,15 @@ virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, * parent_host value on success (>= 0), -1 otherwise. */ static int -virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr parent) +virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr obj)
Here too. ACK Erik
A virNodeDeviceObjPtr is an @obj A virNodeDeviceObjListPtr is a @devs A virNodeDevicePtr is a @device Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 74 ++++++++++++++++++------------------ src/node_device/node_device_hal.c | 38 +++++++++--------- src/node_device/node_device_udev.c | 48 +++++++++++------------ 3 files changed, 78 insertions(+), 82 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 760d73a..8153e21 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -260,7 +260,7 @@ nodeDeviceLookupByName(virConnectPtr conn, { virNodeDeviceObjPtr obj; virNodeDeviceDefPtr def; - virNodeDevicePtr ret = NULL; + virNodeDevicePtr device = NULL; if (!(obj = nodeDeviceObjFindByName(name))) return NULL; @@ -269,16 +269,16 @@ nodeDeviceLookupByName(virConnectPtr conn, if (virNodeDeviceLookupByNameEnsureACL(conn, def) < 0) goto cleanup; - if ((ret = virGetNodeDevice(conn, name))) { - if (VIR_STRDUP(ret->parent, def->parent) < 0) { - virObjectUnref(ret); - ret = NULL; + if ((device = virGetNodeDevice(conn, name))) { + if (VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + device = NULL; } } cleanup: virNodeDeviceObjUnlock(obj); - return ret; + return device; } @@ -293,7 +293,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, virNodeDevCapsDefPtr cap = NULL; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; - virNodeDevicePtr dev = NULL; + virNodeDevicePtr device = NULL; virCheckFlags(0, NULL); @@ -316,10 +316,10 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) goto error; - if ((dev = virGetNodeDevice(conn, def->name))) { - if (VIR_STRDUP(dev->parent, def->parent) < 0) { - virObjectUnref(dev); - dev = NULL; + if ((device = virGetNodeDevice(conn, def->name))) { + if (VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + device = NULL; } } virNodeDeviceObjUnlock(obj); @@ -335,7 +335,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, out: nodeDeviceUnlock(); - return dev; + return device; error: virNodeDeviceObjUnlock(obj); @@ -344,7 +344,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, char * -nodeDeviceGetXMLDesc(virNodeDevicePtr dev, +nodeDeviceGetXMLDesc(virNodeDevicePtr device, unsigned int flags) { virNodeDeviceObjPtr obj; @@ -353,11 +353,11 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, virCheckFlags(0, NULL); - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return NULL; def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, def) < 0) + if (virNodeDeviceGetXMLDescEnsureACL(device->conn, def) < 0) goto cleanup; if (nodeDeviceUpdateDriverName(def) < 0) @@ -375,17 +375,17 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, char * -nodeDeviceGetParent(virNodeDevicePtr dev) +nodeDeviceGetParent(virNodeDevicePtr device) { virNodeDeviceObjPtr obj; virNodeDeviceDefPtr def; char *ret = NULL; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return NULL; def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceGetParentEnsureACL(dev->conn, def) < 0) + if (virNodeDeviceGetParentEnsureACL(device->conn, def) < 0) goto cleanup; if (def->parent) { @@ -403,7 +403,7 @@ nodeDeviceGetParent(virNodeDevicePtr dev) int -nodeDeviceNumOfCaps(virNodeDevicePtr dev) +nodeDeviceNumOfCaps(virNodeDevicePtr device) { virNodeDeviceObjPtr obj; virNodeDeviceDefPtr def; @@ -411,11 +411,11 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev) int ncaps = 0; int ret = -1; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceNumOfCapsEnsureACL(dev->conn, def) < 0) + if (virNodeDeviceNumOfCapsEnsureACL(device->conn, def) < 0) goto cleanup; for (caps = def->caps; caps; caps = caps->next) { @@ -442,7 +442,7 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev) int -nodeDeviceListCaps(virNodeDevicePtr dev, +nodeDeviceListCaps(virNodeDevicePtr device, char **const names, int maxnames) { @@ -452,11 +452,11 @@ nodeDeviceListCaps(virNodeDevicePtr dev, int ncaps = 0; int ret = -1; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceListCapsEnsureACL(dev->conn, def) < 0) + if (virNodeDeviceListCapsEnsureACL(device->conn, def) < 0) goto cleanup; for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) { @@ -530,7 +530,7 @@ nodeDeviceFindNewDevice(virConnectPtr conn, const char *wwnn, const char *wwpn) { - virNodeDevicePtr dev = NULL; + virNodeDevicePtr device = NULL; time_t start = 0, now = 0; /* The thread that creates the device takes the driver lock, so we @@ -546,9 +546,9 @@ nodeDeviceFindNewDevice(virConnectPtr conn, virWaitForDevices(); - dev = nodeDeviceLookupSCSIHostByWWN(conn, wwnn, wwpn, 0); + device = nodeDeviceLookupSCSIHostByWWN(conn, wwnn, wwpn, 0); - if (dev != NULL) + if (device != NULL) break; sleep(5); @@ -558,7 +558,7 @@ nodeDeviceFindNewDevice(virConnectPtr conn, nodeDeviceLock(); - return dev; + return device; } @@ -570,7 +570,7 @@ nodeDeviceCreateXML(virConnectPtr conn, virNodeDeviceDefPtr def = NULL; char *wwnn = NULL, *wwpn = NULL; int parent_host = -1; - virNodeDevicePtr dev = NULL; + virNodeDevicePtr device = NULL; const char *virt_type = NULL; virCheckFlags(0, NULL); @@ -594,11 +594,11 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_CREATE) < 0) goto cleanup; - dev = nodeDeviceFindNewDevice(conn, wwnn, wwpn); + device = nodeDeviceFindNewDevice(conn, wwnn, wwpn); /* We don't check the return value, because one way or another, * we're returning what we get... */ - if (dev == NULL) + if (device == NULL) virReportError(VIR_ERR_NO_NODE_DEVICE, _("no node device for '%s' with matching " "wwnn '%s' and wwpn '%s'"), @@ -608,12 +608,12 @@ nodeDeviceCreateXML(virConnectPtr conn, virNodeDeviceDefFree(def); VIR_FREE(wwnn); VIR_FREE(wwpn); - return dev; + return device; } int -nodeDeviceDestroy(virNodeDevicePtr dev) +nodeDeviceDestroy(virNodeDevicePtr device) { int ret = -1; virNodeDeviceObjPtr obj = NULL; @@ -621,13 +621,13 @@ nodeDeviceDestroy(virNodeDevicePtr dev) char *wwnn = NULL, *wwpn = NULL; int parent_host = -1; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; def = virNodeDeviceObjGetDef(obj); nodeDeviceLock(); - if (virNodeDeviceDestroyEnsureACL(dev->conn, def) < 0) + if (virNodeDeviceDestroyEnsureACL(device->conn, def) < 0) goto cleanup; if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) @@ -659,7 +659,7 @@ nodeDeviceDestroy(virNodeDevicePtr dev) int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, - virNodeDevicePtr dev, + virNodeDevicePtr device, int eventID, virConnectNodeDeviceEventGenericCallback callback, void *opaque, @@ -671,7 +671,7 @@ nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, goto cleanup; if (virNodeDeviceEventStateRegisterID(conn, driver->nodeDeviceEventState, - dev, eventID, callback, + device, eventID, callback, opaque, freecb, &callbackID) < 0) callbackID = -1; cleanup: diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index c354cd3..0d9ec18 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -449,7 +449,7 @@ dev_create(const char *udi) { LibHalContext *ctx; char *parent_key = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def = NULL; virNodeDeviceDefPtr objdef; const char *name = hal_name(udi); @@ -482,15 +482,15 @@ dev_create(const char *udi) /* Some devices don't have a path in sysfs, so ignore failure */ (void)get_str_prop(ctx, udi, "linux.sysfs_path", &devicePath); - if (!(dev = virNodeDeviceObjAssignDef(&driver->devs, def))) { + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) { VIR_FREE(devicePath); goto failure; } - objdef = virNodeDeviceObjGetDef(dev); + objdef = virNodeDeviceObjGetDef(obj); objdef->sysfs_path = devicePath; - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); nodeDeviceUnlock(); return; @@ -506,21 +506,21 @@ static void dev_refresh(const char *udi) { const char *name = hal_name(udi); - virNodeDeviceObjPtr dev; + virNodeDeviceObjPtr obj; nodeDeviceLock(); - dev = virNodeDeviceObjFindByName(&driver->devs, name); - if (dev) { + obj = virNodeDeviceObjFindByName(&driver->devs, name); + if (obj) { /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, dev); + virNodeDeviceObjRemove(&driver->devs, obj); } else { VIR_DEBUG("no device named %s", name); } nodeDeviceUnlock(); - if (dev) + if (obj) dev_create(udi); } @@ -538,14 +538,14 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, const char *udi) { const char *name = hal_name(udi); - virNodeDeviceObjPtr dev; + virNodeDeviceObjPtr obj; nodeDeviceLock(); - dev = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(&driver->devs, name); VIR_DEBUG("%s", name); - if (dev) { - virNodeDeviceObjRemove(&driver->devs, dev); - virNodeDeviceObjFree(dev); + if (obj) { + virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjFree(obj); } else { VIR_DEBUG("no device named %s", name); } @@ -558,17 +558,17 @@ device_cap_added(LibHalContext *ctx, const char *udi, const char *cap) { const char *name = hal_name(udi); - virNodeDeviceObjPtr dev; + virNodeDeviceObjPtr obj; virNodeDeviceDefPtr def; nodeDeviceLock(); - dev = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(&driver->devs, name); nodeDeviceUnlock(); VIR_DEBUG("%s %s", cap, name); - if (dev) { - def = virNodeDeviceObjGetDef(dev); + if (obj) { + def = virNodeDeviceObjGetDef(obj); (void)gather_capability(ctx, udi, cap, &def->caps); - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); } else { VIR_DEBUG("no device named %s", name); } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 819e4e7..708bc9a 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1312,20 +1312,18 @@ udevGetDeviceDetails(struct udev_device *device, static int udevRemoveOneDevice(struct udev_device *device) { - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; virObjectEventPtr event = NULL; const char *name = NULL; name = udev_device_get_syspath(device); - dev = virNodeDeviceObjFindBySysfsPath(&driver->devs, name); - - if (!dev) { + if (!(obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, name))) { VIR_DEBUG("Failed to find device to remove that has udev name '%s'", name); return -1; } - def = virNodeDeviceObjGetDef(dev); + def = virNodeDeviceObjGetDef(obj); event = virNodeDeviceEventLifecycleNew(def->name, VIR_NODE_DEVICE_EVENT_DELETED, @@ -1333,8 +1331,8 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); - virNodeDeviceObjRemove(&driver->devs, dev); - virNodeDeviceObjFree(dev); + virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjFree(obj); if (event) virObjectEventStateQueue(driver->nodeDeviceEventState, event); @@ -1348,7 +1346,7 @@ udevSetParent(struct udev_device *device, { struct udev_device *parent_device = NULL; const char *parent_sysfs_path = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr objdef; int ret = -1; @@ -1367,15 +1365,14 @@ udevSetParent(struct udev_device *device, goto cleanup; } - dev = virNodeDeviceObjFindBySysfsPath(&driver->devs, - parent_sysfs_path); - if (dev != NULL) { - objdef = virNodeDeviceObjGetDef(dev); + if ((obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, + parent_sysfs_path))) { + objdef = virNodeDeviceObjGetDef(obj); if (VIR_STRDUP(def->parent, objdef->name) < 0) { - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); goto cleanup; } - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); if (VIR_STRDUP(def->parent_sysfs_path, parent_sysfs_path) < 0) goto cleanup; @@ -1397,7 +1394,7 @@ static int udevAddOneDevice(struct udev_device *device) { virNodeDeviceDefPtr def = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr objdef; virObjectEventPtr event = NULL; bool new_device = true; @@ -1427,18 +1424,17 @@ udevAddOneDevice(struct udev_device *device) if (udevSetParent(device, def) != 0) goto cleanup; - dev = virNodeDeviceObjFindByName(&driver->devs, def->name); - if (dev) { - virNodeDeviceObjUnlock(dev); + obj = virNodeDeviceObjFindByName(&driver->devs, def->name); + if (obj) { + virNodeDeviceObjUnlock(obj); new_device = false; } /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - dev = virNodeDeviceObjAssignDef(&driver->devs, def); - if (dev == NULL) + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) goto cleanup; - objdef = virNodeDeviceObjGetDef(dev); + objdef = virNodeDeviceObjGetDef(obj); if (new_device) event = virNodeDeviceEventLifecycleNew(objdef->name, @@ -1447,7 +1443,7 @@ udevAddOneDevice(struct udev_device *device) else event = virNodeDeviceEventUpdateNew(objdef->name); - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); ret = 0; @@ -1710,7 +1706,7 @@ static int udevSetupSystemDev(void) { virNodeDeviceDefPtr def = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; int ret = -1; if (VIR_ALLOC(def) < 0) @@ -1726,11 +1722,11 @@ udevSetupSystemDev(void) udevGetDMIData(&def->caps->data.system); #endif - dev = virNodeDeviceObjAssignDef(&driver->devs, def); - if (dev == NULL) + obj = virNodeDeviceObjAssignDef(&driver->devs, def); + if (obj == NULL) goto cleanup; - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); ret = 0; -- 2.9.4
[...]
@@ -506,21 +506,21 @@ static void dev_refresh(const char *udi) { const char *name = hal_name(udi); - virNodeDeviceObjPtr dev; + virNodeDeviceObjPtr obj;
nodeDeviceLock(); - dev = virNodeDeviceObjFindByName(&driver->devs, name); - if (dev) { + obj = virNodeDeviceObjFindByName(&driver->devs, name); + if (obj) {
somewhere further down the changes you went ahead and put this directly into the if clause, so the same could be done on all the relevant places. [...]
- dev = virNodeDeviceObjFindByName(&driver->devs, def->name); - if (dev) { - virNodeDeviceObjUnlock(dev); + obj = virNodeDeviceObjFindByName(&driver->devs, def->name); + if (obj) {
Here as well. [...]
- if (dev == NULL) + obj = virNodeDeviceObjAssignDef(&driver->devs, def); + if (obj == NULL)
Here as well. ACK Erik
Create an allocator for the virNodeDeviceObjPtr - include setting up the mutex, saving the virNodeDeviceDefPtr, and locking the return object. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 3aff5ca..3e88daa 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -33,6 +33,27 @@ VIR_LOG_INIT("conf.virnodedeviceobj"); +static virNodeDeviceObjPtr +virNodeDeviceObjNew(virNodeDeviceDefPtr def) +{ + virNodeDeviceObjPtr obj; + + if (VIR_ALLOC(obj) < 0) + return NULL; + + if (virMutexInit(&obj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot initialize mutex")); + VIR_FREE(obj); + return NULL; + } + virNodeDeviceObjLock(obj); + obj->def = def; + + return obj; +} + + virNodeDeviceDefPtr virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj) { @@ -276,26 +297,17 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, return obj; } - if (VIR_ALLOC(obj) < 0) + if (!(obj = virNodeDeviceObjNew(def))) return NULL; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); - return NULL; - } - virNodeDeviceObjLock(obj); - if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) { + obj->def = NULL; virNodeDeviceObjUnlock(obj); virNodeDeviceObjFree(obj); return NULL; } - obj->def = def; return obj; - } -- 2.9.4
On Sat, Jun 03, 2017 at 09:11:55AM -0400, John Ferlan wrote:
Create an allocator for the virNodeDeviceObjPtr - include setting up the mutex, saving the virNodeDeviceDefPtr, and locking the return object.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
ACK Erik
In preparation to make things private, make the ->devs be pointers to a virNodeDeviceObjList and then manage everything inside virnodedeviceobj Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 13 ++++++++++++- src/conf/virnodedeviceobj.h | 5 ++++- src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 14 +++++++------- src/node_device/node_device_hal.c | 19 +++++++++++-------- src/node_device/node_device_udev.c | 19 +++++++++++-------- src/test/test_driver.c | 29 +++++++++++++++-------------- 7 files changed, 61 insertions(+), 39 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 3e88daa..4a25d95 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -274,6 +274,17 @@ virNodeDeviceObjFree(virNodeDeviceObjPtr obj) } +virNodeDeviceObjListPtr +virNodeDeviceObjListNew(void) +{ + virNodeDeviceObjListPtr devs; + + if (VIR_ALLOC(devs) < 0) + return NULL; + return devs; +} + + void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) { @@ -281,7 +292,7 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) for (i = 0; i < devs->count; i++) virNodeDeviceObjFree(devs->objs[i]); VIR_FREE(devs->objs); - devs->count = 0; + VIR_FREE(devs); } diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 9bc02ee..77250a0 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -32,7 +32,7 @@ typedef virNodeDeviceDriverState *virNodeDeviceDriverStatePtr; struct _virNodeDeviceDriverState { virMutex lock; - virNodeDeviceObjList devs; /* currently-known devices */ + virNodeDeviceObjListPtr devs; /* currently-known devices */ void *privateData; /* driver-specific private data */ /* Immutable pointer, self-locking APIs */ @@ -68,6 +68,9 @@ virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs, void virNodeDeviceObjFree(virNodeDeviceObjPtr dev); +virNodeDeviceObjListPtr +virNodeDeviceObjListNew(void); + void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4a10508..77f56c2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -965,6 +965,7 @@ virNodeDeviceObjGetNames; virNodeDeviceObjGetParentHost; virNodeDeviceObjListExport; virNodeDeviceObjListFree; +virNodeDeviceObjListNew; virNodeDeviceObjLock; virNodeDeviceObjNumOfDevices; virNodeDeviceObjRemove; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 8153e21..fc9d0b0 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -182,7 +182,7 @@ nodeNumOfDevices(virConnectPtr conn, virCheckFlags(0, -1); nodeDeviceLock(); - ndevs = virNodeDeviceObjNumOfDevices(&driver->devs, conn, cap, + ndevs = virNodeDeviceObjNumOfDevices(driver->devs, conn, cap, virNodeNumOfDevicesCheckACL); nodeDeviceUnlock(); @@ -205,7 +205,7 @@ nodeListDevices(virConnectPtr conn, virCheckFlags(0, -1); nodeDeviceLock(); - nnames = virNodeDeviceObjGetNames(&driver->devs, conn, + nnames = virNodeDeviceObjGetNames(driver->devs, conn, virNodeListDevicesCheckACL, cap, names, maxnames); nodeDeviceUnlock(); @@ -227,7 +227,7 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, return -1; nodeDeviceLock(); - ret = virNodeDeviceObjListExport(conn, &driver->devs, devices, + ret = virNodeDeviceObjListExport(conn, driver->devs, devices, virConnectListAllNodeDevicesCheckACL, flags); nodeDeviceUnlock(); @@ -241,7 +241,7 @@ nodeDeviceObjFindByName(const char *name) virNodeDeviceObjPtr obj; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(driver->devs, name); nodeDeviceUnlock(); if (!obj) { @@ -289,7 +289,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, unsigned int flags) { size_t i; - virNodeDeviceObjListPtr devs = &driver->devs; + virNodeDeviceObjListPtr devs = driver->devs; virNodeDevCapsDefPtr cap = NULL; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; @@ -587,7 +587,7 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup; - if ((parent_host = virNodeDeviceObjGetParentHost(&driver->devs, def, + if ((parent_host = virNodeDeviceObjGetParentHost(driver->devs, def, CREATE_DEVICE)) < 0) goto cleanup; @@ -639,7 +639,7 @@ nodeDeviceDestroy(virNodeDevicePtr device) * or parent_fabric_wwn) and drop the object lock. */ virNodeDeviceObjUnlock(obj); obj = NULL; - if ((parent_host = virNodeDeviceObjGetParentHost(&driver->devs, def, + if ((parent_host = virNodeDeviceObjGetParentHost(driver->devs, def, EXISTING_DEVICE)) < 0) goto cleanup; diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 0d9ec18..f02fbe4 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -482,7 +482,7 @@ dev_create(const char *udi) /* Some devices don't have a path in sysfs, so ignore failure */ (void)get_str_prop(ctx, udi, "linux.sysfs_path", &devicePath); - if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) { + if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) { VIR_FREE(devicePath); goto failure; } @@ -509,12 +509,12 @@ dev_refresh(const char *udi) virNodeDeviceObjPtr obj; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(driver->devs, name); if (obj) { /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjRemove(driver->devs, obj); } else { VIR_DEBUG("no device named %s", name); } @@ -541,10 +541,10 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, virNodeDeviceObjPtr obj; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(driver->devs, name); VIR_DEBUG("%s", name); if (obj) { - virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjRemove(driver->devs, obj); virNodeDeviceObjFree(obj); } else { VIR_DEBUG("no device named %s", name); @@ -562,7 +562,7 @@ device_cap_added(LibHalContext *ctx, virNodeDeviceDefPtr def; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(driver->devs, name); nodeDeviceUnlock(); VIR_DEBUG("%s %s", cap, name); if (obj) { @@ -627,6 +627,9 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, } nodeDeviceLock(); + if (!(driver->devs = virNodeDeviceObjListNew())) + goto failure; + dbus_error_init(&err); if (!(sysbus = virDBusGetSystemBus())) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -701,7 +704,7 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, _("%s: %s"), err.name, err.message); dbus_error_free(&err); } - virNodeDeviceObjListFree(&driver->devs); + virNodeDeviceObjListFree(driver->devs); if (hal_ctx) (void)libhal_ctx_free(hal_ctx); nodeDeviceUnlock(); @@ -717,7 +720,7 @@ nodeStateCleanup(void) if (driver) { nodeDeviceLock(); LibHalContext *hal_ctx = DRV_STATE_HAL_CTX(driver); - virNodeDeviceObjListFree(&driver->devs); + virNodeDeviceObjListFree(driver->devs); (void)libhal_ctx_shutdown(hal_ctx, NULL); (void)libhal_ctx_free(hal_ctx); nodeDeviceUnlock(); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 708bc9a..40f12e3 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1318,7 +1318,7 @@ udevRemoveOneDevice(struct udev_device *device) const char *name = NULL; name = udev_device_get_syspath(device); - if (!(obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, name))) { + if (!(obj = virNodeDeviceObjFindBySysfsPath(driver->devs, name))) { VIR_DEBUG("Failed to find device to remove that has udev name '%s'", name); return -1; @@ -1331,7 +1331,7 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); - virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjRemove(driver->devs, obj); virNodeDeviceObjFree(obj); if (event) @@ -1365,7 +1365,7 @@ udevSetParent(struct udev_device *device, goto cleanup; } - if ((obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, + if ((obj = virNodeDeviceObjFindBySysfsPath(driver->devs, parent_sysfs_path))) { objdef = virNodeDeviceObjGetDef(obj); if (VIR_STRDUP(def->parent, objdef->name) < 0) { @@ -1424,7 +1424,7 @@ udevAddOneDevice(struct udev_device *device) if (udevSetParent(device, def) != 0) goto cleanup; - obj = virNodeDeviceObjFindByName(&driver->devs, def->name); + obj = virNodeDeviceObjFindByName(driver->devs, def->name); if (obj) { virNodeDeviceObjUnlock(obj); new_device = false; @@ -1432,7 +1432,7 @@ udevAddOneDevice(struct udev_device *device) /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) + if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) goto cleanup; objdef = virNodeDeviceObjGetDef(obj); @@ -1586,7 +1586,7 @@ nodeStateCleanup(void) if (udev != NULL) udev_unref(udev); - virNodeDeviceObjListFree(&driver->devs); + virNodeDeviceObjListFree(driver->devs); nodeDeviceUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -1722,8 +1722,7 @@ udevSetupSystemDev(void) udevGetDMIData(&def->caps->data.system); #endif - obj = virNodeDeviceObjAssignDef(&driver->devs, def); - if (obj == NULL) + if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) goto cleanup; virNodeDeviceObjUnlock(obj); @@ -1792,6 +1791,10 @@ nodeStateInitialize(bool privileged, driver->privateData = priv; nodeDeviceLock(); + + if (!(driver->devs = virNodeDeviceObjListNew())) + goto cleanup; + driver->nodeDeviceEventState = virObjectEventStateNew(); if (udevPCITranslateInit(privileged) < 0) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e91dfa3..6a74368 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -101,7 +101,7 @@ struct _testDriver { bool transaction_running; virInterfaceObjListPtr backupIfaces; virStoragePoolObjList pools; - virNodeDeviceObjList devs; + virNodeDeviceObjListPtr devs; int numCells; testCell cells[MAX_CELLS]; size_t numAuths; @@ -152,7 +152,7 @@ testDriverFree(testDriverPtr driver) virObjectUnref(driver->caps); virObjectUnref(driver->xmlopt); virObjectUnref(driver->domains); - virNodeDeviceObjListFree(&driver->devs); + virNodeDeviceObjListFree(driver->devs); virObjectUnref(driver->networks); virInterfaceObjListFree(driver->ifaces); virStoragePoolObjListFree(&driver->pools); @@ -418,7 +418,8 @@ testDriverNew(void) !(ret->eventState = virObjectEventStateNew()) || !(ret->ifaces = virInterfaceObjListNew()) || !(ret->domains = virDomainObjListNew()) || - !(ret->networks = virNetworkObjListNew())) + !(ret->networks = virNetworkObjListNew()) || + !(ret->devs = virNodeDeviceObjListNew())) goto error; virAtomicIntSet(&ret->nextDomID, 1); @@ -1169,7 +1170,7 @@ testParseNodedevs(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virNodeDeviceObjAssignDef(&privconn->devs, def))) { + if (!(obj = virNodeDeviceObjAssignDef(privconn->devs, def))) { virNodeDeviceDefFree(def); goto error; } @@ -4519,7 +4520,7 @@ testDestroyVport(testDriverPtr privconn, * * Reaching across the boundaries of space and time into the * Node Device in order to remove */ - if (!(obj = virNodeDeviceObjFindByName(&privconn->devs, "scsi_host12"))) { + if (!(obj = virNodeDeviceObjFindByName(privconn->devs, "scsi_host12"))) { virReportError(VIR_ERR_NO_NODE_DEVICE, "%s", _("no node device with matching name 'scsi_host12'")); return -1; @@ -4529,7 +4530,7 @@ testDestroyVport(testDriverPtr privconn, VIR_NODE_DEVICE_EVENT_DELETED, 0); - virNodeDeviceObjRemove(&privconn->devs, obj); + virNodeDeviceObjRemove(privconn->devs, obj); virNodeDeviceObjFree(obj); obj = NULL; @@ -5262,7 +5263,7 @@ testNodeDeviceObjFindByName(testDriverPtr driver, virNodeDeviceObjPtr obj; testDriverLock(driver); - obj = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(driver->devs, name); testDriverUnlock(driver); if (!obj) @@ -5285,7 +5286,7 @@ testNodeNumOfDevices(virConnectPtr conn, virCheckFlags(0, -1); testDriverLock(driver); - ndevs = virNodeDeviceObjNumOfDevices(&driver->devs, conn, cap, NULL); + ndevs = virNodeDeviceObjNumOfDevices(driver->devs, conn, cap, NULL); testDriverUnlock(driver); return ndevs; @@ -5305,7 +5306,7 @@ testNodeListDevices(virConnectPtr conn, virCheckFlags(0, -1); testDriverLock(driver); - nnames = virNodeDeviceObjGetNames(&driver->devs, conn, NULL, + nnames = virNodeDeviceObjGetNames(driver->devs, conn, NULL, cap, names, maxnames); testDriverUnlock(driver); @@ -5452,7 +5453,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, * using the scsi_host11 definition, changing the name and the * scsi_host capability fields before calling virNodeDeviceAssignDef * to add the def to the node device objects list. */ - if (!(objcopy = virNodeDeviceObjFindByName(&driver->devs, "scsi_host11"))) + if (!(objcopy = virNodeDeviceObjFindByName(driver->devs, "scsi_host11"))) goto cleanup; xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy)); @@ -5492,7 +5493,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, caps = caps->next; } - if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) + if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) goto cleanup; def = NULL; objdef = virNodeDeviceObjGetDef(obj); @@ -5537,7 +5538,7 @@ testNodeDeviceCreateXML(virConnectPtr conn, /* Unlike the "real" code we don't need the parent_host in order to * call virVHBAManageVport, but still let's make sure the code finds * something valid and no one messed up the mock environment. */ - if (virNodeDeviceObjGetParentHost(&driver->devs, def, CREATE_DEVICE) < 0) + if (virNodeDeviceObjGetParentHost(driver->devs, def, CREATE_DEVICE) < 0) goto cleanup; /* In the real code, we'd call virVHBAManageVport followed by @@ -5598,7 +5599,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) /* We do this just for basic validation, but also avoid finding a * vport capable HBA if for some reason our vHBA doesn't exist */ - if (virNodeDeviceObjGetParentHost(&driver->devs, def, + if (virNodeDeviceObjGetParentHost(driver->devs, def, EXISTING_DEVICE) < 0) { obj = NULL; goto cleanup; @@ -5609,7 +5610,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) 0); virNodeDeviceObjLock(obj); - virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjRemove(driver->devs, obj); virNodeDeviceObjFree(obj); obj = NULL; -- 2.9.4
On Sat, Jun 03, 2017 at 09:11:56AM -0400, John Ferlan wrote:
In preparation to make things private, make the ->devs be pointers to a virNodeDeviceObjList and then manage everything inside virnodedeviceobj
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 13 ++++++++++++- src/conf/virnodedeviceobj.h | 5 ++++- src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 14 +++++++------- src/node_device/node_device_hal.c | 19 +++++++++++-------- src/node_device/node_device_udev.c | 19 +++++++++++-------- src/test/test_driver.c | 29 +++++++++++++++-------------- 7 files changed, 61 insertions(+), 39 deletions(-)
ACK Erik
Ensure that any function that walks the node device object list is prefixed by virNodeDeviceObjList. Also, modify the @filter param name for virNodeDeviceObjListExport to be @aclfilter. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 109 ++++++++++++++++++----------------- src/conf/virnodedeviceobj.h | 42 +++++++------- src/libvirt_private.syms | 14 ++--- src/node_device/node_device_driver.c | 20 +++---- src/node_device/node_device_hal.c | 12 ++-- src/node_device/node_device_udev.c | 14 ++--- src/test/test_driver.c | 28 ++++----- 7 files changed, 121 insertions(+), 118 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 4a25d95..fab1cfd 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -166,8 +166,8 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj) virNodeDeviceObjPtr -virNodeDeviceObjFindBySysfsPath(virNodeDeviceObjListPtr devs, - const char *sysfs_path) +virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, + const char *sysfs_path) { size_t i; @@ -185,8 +185,8 @@ virNodeDeviceObjFindBySysfsPath(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr -virNodeDeviceObjFindByName(virNodeDeviceObjListPtr devs, - const char *name) +virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, + const char *name) { size_t i; @@ -202,9 +202,9 @@ virNodeDeviceObjFindByName(virNodeDeviceObjListPtr devs, static virNodeDeviceObjPtr -virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs, - const char *parent_wwnn, - const char *parent_wwpn) +virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, + const char *parent_wwnn, + const char *parent_wwpn) { size_t i; @@ -224,8 +224,8 @@ virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs, static virNodeDeviceObjPtr -virNodeDeviceFindByFabricWWN(virNodeDeviceObjListPtr devs, - const char *parent_fabric_wwn) +virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs, + const char *parent_fabric_wwn) { size_t i; @@ -244,8 +244,8 @@ virNodeDeviceFindByFabricWWN(virNodeDeviceObjListPtr devs, static virNodeDeviceObjPtr -virNodeDeviceFindByCap(virNodeDeviceObjListPtr devs, - const char *cap) +virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, + const char *cap) { size_t i; @@ -297,12 +297,12 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) virNodeDeviceObjPtr -virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def) +virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, + virNodeDeviceDefPtr def) { virNodeDeviceObjPtr obj; - if ((obj = virNodeDeviceObjFindByName(devs, def->name))) { + if ((obj = virNodeDeviceObjListFindByName(devs, def->name))) { virNodeDeviceDefFree(obj->def); obj->def = def; return obj; @@ -323,8 +323,8 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, void -virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr obj) +virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, + virNodeDeviceObjPtr obj) { size_t i; @@ -373,14 +373,14 @@ virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr obj) static int -virNodeDeviceGetParentHostByParent(virNodeDeviceObjListPtr devs, - const char *dev_name, - const char *parent_name) +virNodeDeviceObjListGetParentHostByParent(virNodeDeviceObjListPtr devs, + const char *dev_name, + const char *parent_name) { virNodeDeviceObjPtr obj = NULL; int ret; - if (!(obj = virNodeDeviceObjFindByName(devs, parent_name))) { + if (!(obj = virNodeDeviceObjListFindByName(devs, parent_name))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); @@ -396,15 +396,16 @@ virNodeDeviceGetParentHostByParent(virNodeDeviceObjListPtr devs, static int -virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs, - const char *dev_name, - const char *parent_wwnn, - const char *parent_wwpn) +virNodeDeviceObjListGetParentHostByWWNs(virNodeDeviceObjListPtr devs, + const char *dev_name, + const char *parent_wwnn, + const char *parent_wwpn) { virNodeDeviceObjPtr obj = NULL; int ret; - if (!(obj = virNodeDeviceFindByWWNs(devs, parent_wwnn, parent_wwpn))) { + if (!(obj = virNodeDeviceObjListFindByWWNs(devs, parent_wwnn, + parent_wwpn))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); @@ -420,14 +421,14 @@ virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs, static int -virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, - const char *dev_name, - const char *parent_fabric_wwn) +virNodeDeviceObjListGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, + const char *dev_name, + const char *parent_fabric_wwn) { virNodeDeviceObjPtr obj = NULL; int ret; - if (!(obj = virNodeDeviceFindByFabricWWN(devs, parent_fabric_wwn))) { + if (!(obj = virNodeDeviceObjListFindByFabricWWN(devs, parent_fabric_wwn))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); @@ -443,13 +444,13 @@ virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, static int -virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs) +virNodeDeviceObjListFindVportParentHost(virNodeDeviceObjListPtr devs) { virNodeDeviceObjPtr obj = NULL; const char *cap = virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_VPORTS); int ret; - if (!(obj = virNodeDeviceFindByCap(devs, cap))) { + if (!(obj = virNodeDeviceObjListFindByCap(devs, cap))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find any vport capable device")); return -1; @@ -464,26 +465,26 @@ virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs) int -virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def, - int create) +virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, + virNodeDeviceDefPtr def, + int create) { int parent_host = -1; if (def->parent) { - parent_host = virNodeDeviceGetParentHostByParent(devs, def->name, - def->parent); + parent_host = virNodeDeviceObjListGetParentHostByParent(devs, def->name, + def->parent); } else if (def->parent_wwnn && def->parent_wwpn) { - parent_host = virNodeDeviceGetParentHostByWWNs(devs, def->name, - def->parent_wwnn, - def->parent_wwpn); + parent_host = virNodeDeviceObjListGetParentHostByWWNs(devs, def->name, + def->parent_wwnn, + def->parent_wwpn); } else if (def->parent_fabric_wwn) { parent_host = - virNodeDeviceGetParentHostByFabricWWN(devs, def->name, - def->parent_fabric_wwn); + virNodeDeviceObjListGetParentHostByFabricWWN(devs, def->name, + def->parent_fabric_wwn); } else if (create == CREATE_DEVICE) { /* Try to find a vport capable scsi_host when no parent supplied */ - parent_host = virNodeDeviceFindVportParentHost(devs); + parent_host = virNodeDeviceObjListFindVportParentHost(devs); } return parent_host; @@ -555,10 +556,10 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, int -virNodeDeviceObjNumOfDevices(virNodeDeviceObjListPtr devs, - virConnectPtr conn, - const char *cap, - virNodeDeviceObjListFilter aclfilter) +virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, + virConnectPtr conn, + const char *cap, + virNodeDeviceObjListFilter aclfilter) { size_t i; int ndevs = 0; @@ -577,12 +578,12 @@ virNodeDeviceObjNumOfDevices(virNodeDeviceObjListPtr devs, int -virNodeDeviceObjGetNames(virNodeDeviceObjListPtr devs, - virConnectPtr conn, - virNodeDeviceObjListFilter aclfilter, - const char *cap, - char **const names, - int maxnames) +virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, + virConnectPtr conn, + virNodeDeviceObjListFilter aclfilter, + const char *cap, + char **const names, + int maxnames) { int nnames = 0; size_t i; @@ -646,7 +647,7 @@ int virNodeDeviceObjListExport(virConnectPtr conn, virNodeDeviceObjListPtr devs, virNodeDevicePtr **devices, - virNodeDeviceObjListFilter filter, + virNodeDeviceObjListFilter aclfilter, unsigned int flags) { virNodeDevicePtr *tmp_devices = NULL; @@ -661,7 +662,7 @@ virNodeDeviceObjListExport(virConnectPtr conn, for (i = 0; i < devs->count; i++) { virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDeviceObjLock(obj); - if ((!filter || filter(conn, obj->def)) && + if ((!aclfilter || aclfilter(conn, obj->def)) && virNodeDeviceMatch(obj, flags)) { if (devices) { if (!(device = virGetNodeDevice(conn, obj->def->name)) || diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 77250a0..6194c6c 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -44,25 +44,25 @@ virNodeDeviceDefPtr virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj); virNodeDeviceObjPtr -virNodeDeviceObjFindByName(virNodeDeviceObjListPtr devs, - const char *name); +virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, + const char *name); virNodeDeviceObjPtr -virNodeDeviceObjFindBySysfsPath(virNodeDeviceObjListPtr devs, - const char *sysfs_path) +virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, + const char *sysfs_path) ATTRIBUTE_NONNULL(2); virNodeDeviceObjPtr -virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def); +virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, + virNodeDeviceDefPtr def); void -virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr dev); +virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, + virNodeDeviceObjPtr dev); int -virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def, +virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, + virNodeDeviceDefPtr def, int create); void @@ -85,24 +85,24 @@ typedef bool virNodeDeviceDefPtr def); int -virNodeDeviceObjNumOfDevices(virNodeDeviceObjListPtr devs, - virConnectPtr conn, - const char *cap, - virNodeDeviceObjListFilter aclfilter); +virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, + virConnectPtr conn, + const char *cap, + virNodeDeviceObjListFilter aclfilter); int -virNodeDeviceObjGetNames(virNodeDeviceObjListPtr devs, - virConnectPtr conn, - virNodeDeviceObjListFilter aclfilter, - const char *cap, - char **const names, - int maxnames); +virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, + virConnectPtr conn, + virNodeDeviceObjListFilter aclfilter, + const char *cap, + char **const names, + int maxnames); int virNodeDeviceObjListExport(virConnectPtr conn, virNodeDeviceObjListPtr devobjs, virNodeDevicePtr **devices, - virNodeDeviceObjListFilter filter, + virNodeDeviceObjListFilter aclfilter, unsigned int flags); #endif /* __VIRNODEDEVICEOBJ_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 77f56c2..b511feb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -956,19 +956,19 @@ virNetworkObjUpdateAssignDef; # conf/virnodedeviceobj.h -virNodeDeviceObjAssignDef; -virNodeDeviceObjFindByName; -virNodeDeviceObjFindBySysfsPath; virNodeDeviceObjFree; virNodeDeviceObjGetDef; -virNodeDeviceObjGetNames; -virNodeDeviceObjGetParentHost; +virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; +virNodeDeviceObjListFindByName; +virNodeDeviceObjListFindBySysfsPath; virNodeDeviceObjListFree; +virNodeDeviceObjListGetNames; +virNodeDeviceObjListGetParentHost; virNodeDeviceObjListNew; +virNodeDeviceObjListNumOfDevices; +virNodeDeviceObjListRemove; virNodeDeviceObjLock; -virNodeDeviceObjNumOfDevices; -virNodeDeviceObjRemove; virNodeDeviceObjUnlock; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index fc9d0b0..348539f 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -182,8 +182,8 @@ nodeNumOfDevices(virConnectPtr conn, virCheckFlags(0, -1); nodeDeviceLock(); - ndevs = virNodeDeviceObjNumOfDevices(driver->devs, conn, cap, - virNodeNumOfDevicesCheckACL); + ndevs = virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap, + virNodeNumOfDevicesCheckACL); nodeDeviceUnlock(); return ndevs; @@ -205,9 +205,9 @@ nodeListDevices(virConnectPtr conn, virCheckFlags(0, -1); nodeDeviceLock(); - nnames = virNodeDeviceObjGetNames(driver->devs, conn, - virNodeListDevicesCheckACL, - cap, names, maxnames); + nnames = virNodeDeviceObjListGetNames(driver->devs, conn, + virNodeListDevicesCheckACL, + cap, names, maxnames); nodeDeviceUnlock(); return nnames; @@ -241,7 +241,7 @@ nodeDeviceObjFindByName(const char *name) virNodeDeviceObjPtr obj; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(driver->devs, name); + obj = virNodeDeviceObjListFindByName(driver->devs, name); nodeDeviceUnlock(); if (!obj) { @@ -587,8 +587,8 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup; - if ((parent_host = virNodeDeviceObjGetParentHost(driver->devs, def, - CREATE_DEVICE)) < 0) + if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, + CREATE_DEVICE)) < 0) goto cleanup; if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_CREATE) < 0) @@ -639,8 +639,8 @@ nodeDeviceDestroy(virNodeDevicePtr device) * or parent_fabric_wwn) and drop the object lock. */ virNodeDeviceObjUnlock(obj); obj = NULL; - if ((parent_host = virNodeDeviceObjGetParentHost(driver->devs, def, - EXISTING_DEVICE)) < 0) + if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, + EXISTING_DEVICE)) < 0) goto cleanup; if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_DELETE) < 0) diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index f02fbe4..5521287 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -482,7 +482,7 @@ dev_create(const char *udi) /* Some devices don't have a path in sysfs, so ignore failure */ (void)get_str_prop(ctx, udi, "linux.sysfs_path", &devicePath); - if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) { + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) { VIR_FREE(devicePath); goto failure; } @@ -509,12 +509,12 @@ dev_refresh(const char *udi) virNodeDeviceObjPtr obj; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(driver->devs, name); + obj = virNodeDeviceObjListFindByName(driver->devs, name); if (obj) { /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver->devs, obj); } else { VIR_DEBUG("no device named %s", name); } @@ -541,10 +541,10 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, virNodeDeviceObjPtr obj; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(driver->devs, name); + obj = virNodeDeviceObjListFindByName(driver->devs, name); VIR_DEBUG("%s", name); if (obj) { - virNodeDeviceObjRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver->devs, obj); virNodeDeviceObjFree(obj); } else { VIR_DEBUG("no device named %s", name); @@ -562,7 +562,7 @@ device_cap_added(LibHalContext *ctx, virNodeDeviceDefPtr def; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(driver->devs, name); + obj = virNodeDeviceObjListFindByName(driver->devs, name); nodeDeviceUnlock(); VIR_DEBUG("%s %s", cap, name); if (obj) { diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 40f12e3..a210fe1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1318,7 +1318,7 @@ udevRemoveOneDevice(struct udev_device *device) const char *name = NULL; name = udev_device_get_syspath(device); - if (!(obj = virNodeDeviceObjFindBySysfsPath(driver->devs, name))) { + if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, name))) { VIR_DEBUG("Failed to find device to remove that has udev name '%s'", name); return -1; @@ -1331,7 +1331,7 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); - virNodeDeviceObjRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver->devs, obj); virNodeDeviceObjFree(obj); if (event) @@ -1365,8 +1365,8 @@ udevSetParent(struct udev_device *device, goto cleanup; } - if ((obj = virNodeDeviceObjFindBySysfsPath(driver->devs, - parent_sysfs_path))) { + if ((obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, + parent_sysfs_path))) { objdef = virNodeDeviceObjGetDef(obj); if (VIR_STRDUP(def->parent, objdef->name) < 0) { virNodeDeviceObjUnlock(obj); @@ -1424,7 +1424,7 @@ udevAddOneDevice(struct udev_device *device) if (udevSetParent(device, def) != 0) goto cleanup; - obj = virNodeDeviceObjFindByName(driver->devs, def->name); + obj = virNodeDeviceObjListFindByName(driver->devs, def->name); if (obj) { virNodeDeviceObjUnlock(obj); new_device = false; @@ -1432,7 +1432,7 @@ udevAddOneDevice(struct udev_device *device) /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; objdef = virNodeDeviceObjGetDef(obj); @@ -1722,7 +1722,7 @@ udevSetupSystemDev(void) udevGetDMIData(&def->caps->data.system); #endif - if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; virNodeDeviceObjUnlock(obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6a74368..67fe252 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1170,7 +1170,7 @@ testParseNodedevs(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virNodeDeviceObjAssignDef(privconn->devs, def))) { + if (!(obj = virNodeDeviceObjListAssignDef(privconn->devs, def))) { virNodeDeviceDefFree(def); goto error; } @@ -4520,7 +4520,8 @@ testDestroyVport(testDriverPtr privconn, * * Reaching across the boundaries of space and time into the * Node Device in order to remove */ - if (!(obj = virNodeDeviceObjFindByName(privconn->devs, "scsi_host12"))) { + if (!(obj = virNodeDeviceObjListFindByName(privconn->devs, + "scsi_host12"))) { virReportError(VIR_ERR_NO_NODE_DEVICE, "%s", _("no node device with matching name 'scsi_host12'")); return -1; @@ -4530,7 +4531,7 @@ testDestroyVport(testDriverPtr privconn, VIR_NODE_DEVICE_EVENT_DELETED, 0); - virNodeDeviceObjRemove(privconn->devs, obj); + virNodeDeviceObjListRemove(privconn->devs, obj); virNodeDeviceObjFree(obj); obj = NULL; @@ -5263,7 +5264,7 @@ testNodeDeviceObjFindByName(testDriverPtr driver, virNodeDeviceObjPtr obj; testDriverLock(driver); - obj = virNodeDeviceObjFindByName(driver->devs, name); + obj = virNodeDeviceObjListFindByName(driver->devs, name); testDriverUnlock(driver); if (!obj) @@ -5286,7 +5287,7 @@ testNodeNumOfDevices(virConnectPtr conn, virCheckFlags(0, -1); testDriverLock(driver); - ndevs = virNodeDeviceObjNumOfDevices(driver->devs, conn, cap, NULL); + ndevs = virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap, NULL); testDriverUnlock(driver); return ndevs; @@ -5306,8 +5307,8 @@ testNodeListDevices(virConnectPtr conn, virCheckFlags(0, -1); testDriverLock(driver); - nnames = virNodeDeviceObjGetNames(driver->devs, conn, NULL, - cap, names, maxnames); + nnames = virNodeDeviceObjListGetNames(driver->devs, conn, NULL, + cap, names, maxnames); testDriverUnlock(driver); return nnames; @@ -5453,7 +5454,8 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, * using the scsi_host11 definition, changing the name and the * scsi_host capability fields before calling virNodeDeviceAssignDef * to add the def to the node device objects list. */ - if (!(objcopy = virNodeDeviceObjFindByName(driver->devs, "scsi_host11"))) + if (!(objcopy = virNodeDeviceObjListFindByName(driver->devs, + "scsi_host11"))) goto cleanup; xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy)); @@ -5493,7 +5495,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, caps = caps->next; } - if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; def = NULL; objdef = virNodeDeviceObjGetDef(obj); @@ -5538,7 +5540,7 @@ testNodeDeviceCreateXML(virConnectPtr conn, /* Unlike the "real" code we don't need the parent_host in order to * call virVHBAManageVport, but still let's make sure the code finds * something valid and no one messed up the mock environment. */ - if (virNodeDeviceObjGetParentHost(driver->devs, def, CREATE_DEVICE) < 0) + if (virNodeDeviceObjListGetParentHost(driver->devs, def, CREATE_DEVICE) < 0) goto cleanup; /* In the real code, we'd call virVHBAManageVport followed by @@ -5599,8 +5601,8 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) /* We do this just for basic validation, but also avoid finding a * vport capable HBA if for some reason our vHBA doesn't exist */ - if (virNodeDeviceObjGetParentHost(driver->devs, def, - EXISTING_DEVICE) < 0) { + if (virNodeDeviceObjListGetParentHost(driver->devs, def, + EXISTING_DEVICE) < 0) { obj = NULL; goto cleanup; } @@ -5610,7 +5612,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) 0); virNodeDeviceObjLock(obj); - virNodeDeviceObjRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver->devs, obj); virNodeDeviceObjFree(obj); obj = NULL; -- 2.9.4
On Sat, Jun 03, 2017 at 09:11:57AM -0400, John Ferlan wrote:
Ensure that any function that walks the node device object list is prefixed by virNodeDeviceObjList.
Also, modify the @filter param name for virNodeDeviceObjListExport to be @aclfilter.
Signed-off-by: John Ferlan <jferlan@redhat.com>
One more from the 'not a fan' patches :)... ACK Erik
static virNodeDeviceObjPtr -virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs, - const char *parent_wwnn, - const char *parent_wwpn) +virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, + const char *parent_wwnn, + const char *parent_wwpn)
I assume one would argue that calling this function simply *ByWWN wouldn't reflect the reality and justify the actual need for both wwnn and wwpn.
Create local @obj and @def for the API's rather than referencing the devs->objs[i][->def->]. It'll make future patches easier to read. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 60 ++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index fab1cfd..a9187be 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -172,12 +172,16 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, size_t i; for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); - if ((devs->objs[i]->def->sysfs_path != NULL) && - (STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) { - return devs->objs[i]; + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDeviceDefPtr def; + + virNodeDeviceObjLock(obj); + def = obj->def; + if ((def->sysfs_path != NULL) && + (STREQ(def->sysfs_path, sysfs_path))) { + return obj; } - virNodeDeviceObjUnlock(devs->objs[i]); + virNodeDeviceObjUnlock(obj); } return NULL; @@ -191,10 +195,14 @@ virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, size_t i; for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); - if (STREQ(devs->objs[i]->def->name, name)) - return devs->objs[i]; - virNodeDeviceObjUnlock(devs->objs[i]); + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDeviceDefPtr def; + + virNodeDeviceObjLock(obj); + def = obj->def; + if (STREQ(def->name, name)) + return obj; + virNodeDeviceObjUnlock(obj); } return NULL; @@ -209,14 +217,16 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, size_t i; for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDevCapsDefPtr cap; - virNodeDeviceObjLock(devs->objs[i]); - if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) && + + virNodeDeviceObjLock(obj); + if ((cap = virNodeDeviceFindFCCapDef(obj)) && STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) && STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) && - virNodeDeviceFindVPORTCapDef(devs->objs[i])) - return devs->objs[i]; - virNodeDeviceObjUnlock(devs->objs[i]); + virNodeDeviceFindVPORTCapDef(obj)) + return obj; + virNodeDeviceObjUnlock(obj); } return NULL; @@ -230,13 +240,15 @@ virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs, size_t i; for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDevCapsDefPtr cap; - virNodeDeviceObjLock(devs->objs[i]); - if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) && + + virNodeDeviceObjLock(obj); + if ((cap = virNodeDeviceFindFCCapDef(obj)) && STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) && - virNodeDeviceFindVPORTCapDef(devs->objs[i])) - return devs->objs[i]; - virNodeDeviceObjUnlock(devs->objs[i]); + virNodeDeviceFindVPORTCapDef(obj)) + return obj; + virNodeDeviceObjUnlock(obj); } return NULL; @@ -250,10 +262,12 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, size_t i; for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); - if (virNodeDeviceObjHasCap(devs->objs[i], cap)) - return devs->objs[i]; - virNodeDeviceObjUnlock(devs->objs[i]); + virNodeDeviceObjPtr obj = devs->objs[i]; + + virNodeDeviceObjLock(obj); + if (virNodeDeviceObjHasCap(obj, cap)) + return obj; + virNodeDeviceObjUnlock(obj); } return NULL; -- 2.9.4
On Sat, Jun 03, 2017 at 09:11:58AM -0400, John Ferlan wrote:
Create local @obj and @def for the API's rather than referencing the devs->objs[i][->def->]. It'll make future patches easier to read.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 60 ++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index fab1cfd..a9187be 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -172,12 +172,16 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, size_t i;
for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); - if ((devs->objs[i]->def->sysfs_path != NULL) && - (STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) { - return devs->objs[i]; + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDeviceDefPtr def;
Although with good intension, I think having virNodeDeviceObjPtr obj = foo only will make all the difference, thus dereferencing @def is IMHO not that much more beneficial. ACK with that. Erik
On 06/30/2017 08:06 AM, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:58AM -0400, John Ferlan wrote:
Create local @obj and @def for the API's rather than referencing the devs->objs[i][->def->]. It'll make future patches easier to read.
^^^^^^^^^ [1]
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 60 ++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index fab1cfd..a9187be 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -172,12 +172,16 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, size_t i;
for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); - if ((devs->objs[i]->def->sysfs_path != NULL) && - (STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) { - return devs->objs[i]; + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDeviceDefPtr def;
Although with good intension, I think having virNodeDeviceObjPtr obj = foo only will make all the difference, thus dereferencing @def is IMHO not that much more beneficial.
[1] I'm trying to avoid syntax such as: devs->objs[i]->def->sysfs_path or objs->def->sysfs_path If some day ->def is "owned-by" some vir*Object, then -> def would need to be fetched, e.g. def = virObject*GetDef(obj), so this just makes that future easier. John
ACK with that.
Erik
On Fri, Jun 30, 2017 at 01:08:22PM -0400, John Ferlan wrote:
On 06/30/2017 08:06 AM, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:11:58AM -0400, John Ferlan wrote:
Create local @obj and @def for the API's rather than referencing the devs->objs[i][->def->]. It'll make future patches easier to read.
^^^^^^^^^ [1]
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 60 ++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index fab1cfd..a9187be 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -172,12 +172,16 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, size_t i;
for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); - if ((devs->objs[i]->def->sysfs_path != NULL) && - (STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) { - return devs->objs[i]; + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDeviceDefPtr def;
Although with good intension, I think having virNodeDeviceObjPtr obj = foo only will make all the difference, thus dereferencing @def is IMHO not that much more beneficial.
[1] I'm trying to avoid syntax such as:
devs->objs[i]->def->sysfs_path
or
objs->def->sysfs_path
If some day ->def is "owned-by" some vir*Object, then -> def would need to be fetched, e.g. def = virObject*GetDef(obj), so this just makes that future easier.
Yeah, I'm realizing it was a pointless suggestion, strange I didn't even think about that when I was looking at 10/12, you can disregard my previous comment then...^good point btw. ACK as is. Erik
We're about to move the call to nodeDeviceSysfsGetSCSIHostCaps from node_device_driver into virnodedeviceobj, so move the guts of the code from the driver specific node_device_linux_sysfs into its own API since virnodedeviceobj cannot callback into the driver. Nothing in the code deals with sysfs anyway, as that's hidden by the various virSCSIHost* and virVHBA* utility function calls. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 82 +++++++++++++++++++++++++++++++ src/conf/node_device_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/node_device/node_device_linux_sysfs.c | 77 +---------------------------- 4 files changed, 87 insertions(+), 76 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index e5947e6..503b129 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2483,3 +2483,85 @@ virNodeDeviceDeleteVport(virConnectPtr conn, VIR_FREE(scsi_host_name); return ret; } + + +int +virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host) +{ + char *tmp = NULL; + int ret = -1; + + if ((scsi_host->unique_id = + virSCSIHostGetUniqueId(NULL, scsi_host->host)) < 0) { + VIR_DEBUG("Failed to read unique_id for host%d", scsi_host->host); + scsi_host->unique_id = -1; + } + + VIR_DEBUG("Checking if host%d is an FC HBA", scsi_host->host); + + if (virVHBAPathExists(NULL, scsi_host->host)) { + scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST; + + if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, "port_name"))) { + VIR_WARN("Failed to read WWPN for host%d", scsi_host->host); + goto cleanup; + } + VIR_FREE(scsi_host->wwpn); + VIR_STEAL_PTR(scsi_host->wwpn, tmp); + + if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, "node_name"))) { + VIR_WARN("Failed to read WWNN for host%d", scsi_host->host); + goto cleanup; + } + VIR_FREE(scsi_host->wwnn); + VIR_STEAL_PTR(scsi_host->wwnn, tmp); + + if ((tmp = virVHBAGetConfig(NULL, scsi_host->host, "fabric_name"))) { + VIR_FREE(scsi_host->fabric_wwn); + VIR_STEAL_PTR(scsi_host->fabric_wwn, tmp); + } + } + + if (virVHBAIsVportCapable(NULL, scsi_host->host)) { + scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS; + + if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, + "max_npiv_vports"))) { + VIR_WARN("Failed to read max_npiv_vports for host%d", + scsi_host->host); + goto cleanup; + } + + if (virStrToLong_i(tmp, NULL, 10, &scsi_host->max_vports) < 0) { + VIR_WARN("Failed to parse value of max_npiv_vports '%s'", tmp); + goto cleanup; + } + + VIR_FREE(tmp); + if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, + "npiv_vports_inuse"))) { + VIR_WARN("Failed to read npiv_vports_inuse for host%d", + scsi_host->host); + goto cleanup; + } + + if (virStrToLong_i(tmp, NULL, 10, &scsi_host->vports) < 0) { + VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'", tmp); + goto cleanup; + } + } + + ret = 0; + cleanup: + if (ret < 0) { + /* Clear the two flags in case of producing confusing XML output */ + scsi_host->flags &= ~(VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST | + VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS); + + VIR_FREE(scsi_host->wwnn); + VIR_FREE(scsi_host->wwpn); + VIR_FREE(scsi_host->fabric_wwn); + } + VIR_FREE(tmp); + return ret; +} diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 0a5e731..90c7e1f 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -408,4 +408,7 @@ int virNodeDeviceDeleteVport(virConnectPtr conn, virStorageAdapterFCHostPtr fchost); +int +virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host); + #endif /* __VIR_NODE_DEVICE_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b511feb..33fc9fc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -680,6 +680,7 @@ virNodeDeviceDefParseNode; virNodeDeviceDefParseString; virNodeDeviceDeleteVport; virNodeDeviceGetParentName; +virNodeDeviceGetSCSIHostCaps; virNodeDeviceGetWWNs; diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index e02c384..6f438e5 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -48,82 +48,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs"); int nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host) { - char *tmp = NULL; - int ret = -1; - - if ((scsi_host->unique_id = - virSCSIHostGetUniqueId(NULL, scsi_host->host)) < 0) { - VIR_DEBUG("Failed to read unique_id for host%d", scsi_host->host); - scsi_host->unique_id = -1; - } - - VIR_DEBUG("Checking if host%d is an FC HBA", scsi_host->host); - - if (virVHBAPathExists(NULL, scsi_host->host)) { - scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST; - - if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, "port_name"))) { - VIR_WARN("Failed to read WWPN for host%d", scsi_host->host); - goto cleanup; - } - VIR_FREE(scsi_host->wwpn); - VIR_STEAL_PTR(scsi_host->wwpn, tmp); - - if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, "node_name"))) { - VIR_WARN("Failed to read WWNN for host%d", scsi_host->host); - goto cleanup; - } - VIR_FREE(scsi_host->wwnn); - VIR_STEAL_PTR(scsi_host->wwnn, tmp); - - if ((tmp = virVHBAGetConfig(NULL, scsi_host->host, "fabric_name"))) { - VIR_FREE(scsi_host->fabric_wwn); - VIR_STEAL_PTR(scsi_host->fabric_wwn, tmp); - } - } - - if (virVHBAIsVportCapable(NULL, scsi_host->host)) { - scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS; - - if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, - "max_npiv_vports"))) { - VIR_WARN("Failed to read max_npiv_vports for host%d", - scsi_host->host); - goto cleanup; - } - - if (virStrToLong_i(tmp, NULL, 10, &scsi_host->max_vports) < 0) { - VIR_WARN("Failed to parse value of max_npiv_vports '%s'", tmp); - goto cleanup; - } - - VIR_FREE(tmp); - if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, - "npiv_vports_inuse"))) { - VIR_WARN("Failed to read npiv_vports_inuse for host%d", - scsi_host->host); - goto cleanup; - } - - if (virStrToLong_i(tmp, NULL, 10, &scsi_host->vports) < 0) { - VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'", tmp); - goto cleanup; - } - } - - ret = 0; - cleanup: - if (ret < 0) { - /* Clear the two flags in case of producing confusing XML output */ - scsi_host->flags &= ~(VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST | - VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS); - - VIR_FREE(scsi_host->wwnn); - VIR_FREE(scsi_host->wwpn); - VIR_FREE(scsi_host->fabric_wwn); - } - VIR_FREE(tmp); - return ret; + return virNodeDeviceGetSCSIHostCaps(scsi_host); } -- 2.9.4
On Sat, Jun 03, 2017 at 09:11:59AM -0400, John Ferlan wrote:
We're about to move the call to nodeDeviceSysfsGetSCSIHostCaps from node_device_driver into virnodedeviceobj, so move the guts of the code from the driver specific node_device_linux_sysfs into its own API since virnodedeviceobj cannot callback into the driver.
Nothing in the code deals with sysfs anyway, as that's hidden by the various virSCSIHost* and virVHBA* utility function calls.
ACK Erik
Alter the nodeDeviceLookupSCSIHostByWWN to use the new API in order to find what it's looking for. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 33 +++++++++++++++++++++ src/conf/virnodedeviceobj.h | 5 ++++ src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 56 +++++++++++------------------------- 4 files changed, 55 insertions(+), 40 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index a9187be..fa61a2d 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -274,6 +274,39 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, } +virNodeDeviceObjPtr +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, + const char *wwnn, + const char *wwpn) +{ + size_t i; + + for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDevCapsDefPtr cap; + + virNodeDeviceObjLock(obj); + cap = obj->def->caps; + + while (cap) { + if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { + virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host); + if (cap->data.scsi_host.flags & + VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { + if (STREQ(cap->data.scsi_host.wwnn, wwnn) && + STREQ(cap->data.scsi_host.wwpn, wwpn)) + return obj; + } + } + cap = cap->next; + } + virNodeDeviceObjUnlock(obj); + } + + return NULL; +} + + void virNodeDeviceObjFree(virNodeDeviceObjPtr obj) { diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 6194c6c..6ec5ee7 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -53,6 +53,11 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, ATTRIBUTE_NONNULL(2); virNodeDeviceObjPtr +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, + const char *wwnn, + const char *wwpn); + +virNodeDeviceObjPtr virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 33fc9fc..d415888 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -963,6 +963,7 @@ virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; virNodeDeviceObjListFindByName; virNodeDeviceObjListFindBySysfsPath; +virNodeDeviceObjListFindSCSIHostByWWNs; virNodeDeviceObjListFree; virNodeDeviceObjListGetNames; virNodeDeviceObjListGetParentHost; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 348539f..4a5f168 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -288,9 +288,6 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, const char *wwpn, unsigned int flags) { - size_t i; - virNodeDeviceObjListPtr devs = driver->devs; - virNodeDevCapsDefPtr cap = NULL; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; virNodeDevicePtr device = NULL; @@ -298,48 +295,27 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, virCheckFlags(0, NULL); nodeDeviceLock(); + obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn); + nodeDeviceUnlock(); - for (i = 0; i < devs->count; i++) { - obj = devs->objs[i]; - virNodeDeviceObjLock(obj); - def = virNodeDeviceObjGetDef(obj); - cap = def->caps; - - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - nodeDeviceSysfsGetSCSIHostCaps(&cap->data.scsi_host); - if (cap->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { - if (STREQ(cap->data.scsi_host.wwnn, wwnn) && - STREQ(cap->data.scsi_host.wwpn, wwpn)) { - - if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) - goto error; - - if ((device = virGetNodeDevice(conn, def->name))) { - if (VIR_STRDUP(device->parent, def->parent) < 0) { - virObjectUnref(device); - device = NULL; - } - } - virNodeDeviceObjUnlock(obj); - goto out; - } - } - } - cap = cap->next; - } + if (!obj) + return NULL; - virNodeDeviceObjUnlock(obj); - } + def = virNodeDeviceObjGetDef(obj); - out: - nodeDeviceUnlock(); - return device; + if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) + goto cleanup; - error: + if ((device = virGetNodeDevice(conn, def->name))) { + if (VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + device = NULL; + } + } + + cleanup: virNodeDeviceObjUnlock(obj); - goto out; + return device; } -- 2.9.4
On Sat, Jun 03, 2017 at 09:12:00AM -0400, John Ferlan wrote:
Alter the nodeDeviceLookupSCSIHostByWWN to use the new API in order to find what it's looking for.
Would you mind enhancing the commit message a bit? I assume the new API is virNodeDeviceGetSCSIHostCaps but a random reader would lack the context. It misses the main motivation to move nodeDeviceLookupSCSIHostByWWN out of the driver simply because of the NodeDeviceObj privatization. Also, this got me thinking, whether 9/10 and 10/10 is critical for this series, so I tried to rewrite it without it and somewhere down the road I decided that it was an ugly, pointless waste of time and I like it this way much more. ACK if you enhance the commit message a bit. Erik
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 33 +++++++++++++++++++++ src/conf/virnodedeviceobj.h | 5 ++++ src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 56 +++++++++++------------------------- 4 files changed, 55 insertions(+), 40 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index a9187be..fa61a2d 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -274,6 +274,39 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, }
+virNodeDeviceObjPtr +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, + const char *wwnn, + const char *wwpn) +{ + size_t i; + + for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDevCapsDefPtr cap; + + virNodeDeviceObjLock(obj); + cap = obj->def->caps; + + while (cap) { + if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { + virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host); + if (cap->data.scsi_host.flags & + VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { + if (STREQ(cap->data.scsi_host.wwnn, wwnn) && + STREQ(cap->data.scsi_host.wwpn, wwpn)) + return obj; + } + } + cap = cap->next; + } + virNodeDeviceObjUnlock(obj); + } + + return NULL; +} + + void virNodeDeviceObjFree(virNodeDeviceObjPtr obj) { diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 6194c6c..6ec5ee7 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -53,6 +53,11 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, ATTRIBUTE_NONNULL(2);
virNodeDeviceObjPtr +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, + const char *wwnn, + const char *wwpn); + +virNodeDeviceObjPtr virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 33fc9fc..d415888 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -963,6 +963,7 @@ virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; virNodeDeviceObjListFindByName; virNodeDeviceObjListFindBySysfsPath; +virNodeDeviceObjListFindSCSIHostByWWNs; virNodeDeviceObjListFree; virNodeDeviceObjListGetNames; virNodeDeviceObjListGetParentHost; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 348539f..4a5f168 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -288,9 +288,6 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, const char *wwpn, unsigned int flags) { - size_t i; - virNodeDeviceObjListPtr devs = driver->devs; - virNodeDevCapsDefPtr cap = NULL; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; virNodeDevicePtr device = NULL; @@ -298,48 +295,27 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, virCheckFlags(0, NULL);
nodeDeviceLock(); + obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn); + nodeDeviceUnlock();
- for (i = 0; i < devs->count; i++) { - obj = devs->objs[i]; - virNodeDeviceObjLock(obj); - def = virNodeDeviceObjGetDef(obj); - cap = def->caps; - - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - nodeDeviceSysfsGetSCSIHostCaps(&cap->data.scsi_host); - if (cap->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { - if (STREQ(cap->data.scsi_host.wwnn, wwnn) && - STREQ(cap->data.scsi_host.wwpn, wwpn)) { - - if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) - goto error; - - if ((device = virGetNodeDevice(conn, def->name))) { - if (VIR_STRDUP(device->parent, def->parent) < 0) { - virObjectUnref(device); - device = NULL; - } - } - virNodeDeviceObjUnlock(obj); - goto out; - } - } - } - cap = cap->next; - } + if (!obj) + return NULL;
- virNodeDeviceObjUnlock(obj); - } + def = virNodeDeviceObjGetDef(obj);
- out: - nodeDeviceUnlock(); - return device; + if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) + goto cleanup;
- error: + if ((device = virGetNodeDevice(conn, def->name))) { + if (VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + device = NULL; + } + } + + cleanup: virNodeDeviceObjUnlock(obj); - goto out; + return device; }
-- 2.9.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/03/2017 09:12 AM, Erik Skultety wrote:
On Sat, Jun 03, 2017 at 09:12:00AM -0400, John Ferlan wrote:
Alter the nodeDeviceLookupSCSIHostByWWN to use the new API in order to find what it's looking for.
Would you mind enhancing the commit message a bit? I assume the new API is virNodeDeviceGetSCSIHostCaps but a random reader would lack the context. It misses the main motivation to move nodeDeviceLookupSCSIHostByWWN out of the driver simply because of the NodeDeviceObj privatization. Also, this got me thinking, whether 9/10 and 10/10 is critical for this series, so I tried to rewrite it without it and somewhere down the road I decided that it was an ugly, pointless waste of time and I like it this way much more.
ACK if you enhance the commit message a bit.
Erik
Sure I'll add some details... My first hack at 9/12 wasn't well received either since I moved too much, see: https://www.redhat.com/archives/libvir-list/2017-May/msg00722.html and followups to that. So I took the move and call path of less resistance. The 10/12 change just follows other Lookup functions. It'll get even more interesting soon when you see patch 13. John
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 33 +++++++++++++++++++++ src/conf/virnodedeviceobj.h | 5 ++++ src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 56 +++++++++++------------------------- 4 files changed, 55 insertions(+), 40 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index a9187be..fa61a2d 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -274,6 +274,39 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, }
+virNodeDeviceObjPtr +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, + const char *wwnn, + const char *wwpn) +{ + size_t i; + + for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDevCapsDefPtr cap; + + virNodeDeviceObjLock(obj); + cap = obj->def->caps; + + while (cap) { + if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { + virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host); + if (cap->data.scsi_host.flags & + VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { + if (STREQ(cap->data.scsi_host.wwnn, wwnn) && + STREQ(cap->data.scsi_host.wwpn, wwpn)) + return obj; + } + } + cap = cap->next; + } + virNodeDeviceObjUnlock(obj); + } + + return NULL; +} + + void virNodeDeviceObjFree(virNodeDeviceObjPtr obj) { diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 6194c6c..6ec5ee7 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -53,6 +53,11 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, ATTRIBUTE_NONNULL(2);
virNodeDeviceObjPtr +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, + const char *wwnn, + const char *wwpn); + +virNodeDeviceObjPtr virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 33fc9fc..d415888 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -963,6 +963,7 @@ virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; virNodeDeviceObjListFindByName; virNodeDeviceObjListFindBySysfsPath; +virNodeDeviceObjListFindSCSIHostByWWNs; virNodeDeviceObjListFree; virNodeDeviceObjListGetNames; virNodeDeviceObjListGetParentHost; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 348539f..4a5f168 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -288,9 +288,6 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, const char *wwpn, unsigned int flags) { - size_t i; - virNodeDeviceObjListPtr devs = driver->devs; - virNodeDevCapsDefPtr cap = NULL; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; virNodeDevicePtr device = NULL; @@ -298,48 +295,27 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, virCheckFlags(0, NULL);
nodeDeviceLock(); + obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn); + nodeDeviceUnlock();
- for (i = 0; i < devs->count; i++) { - obj = devs->objs[i]; - virNodeDeviceObjLock(obj); - def = virNodeDeviceObjGetDef(obj); - cap = def->caps; - - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - nodeDeviceSysfsGetSCSIHostCaps(&cap->data.scsi_host); - if (cap->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { - if (STREQ(cap->data.scsi_host.wwnn, wwnn) && - STREQ(cap->data.scsi_host.wwpn, wwpn)) { - - if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) - goto error; - - if ((device = virGetNodeDevice(conn, def->name))) { - if (VIR_STRDUP(device->parent, def->parent) < 0) { - virObjectUnref(device); - device = NULL; - } - } - virNodeDeviceObjUnlock(obj); - goto out; - } - } - } - cap = cap->next; - } + if (!obj) + return NULL;
- virNodeDeviceObjUnlock(obj); - } + def = virNodeDeviceObjGetDef(obj);
- out: - nodeDeviceUnlock(); - return device; + if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) + goto cleanup;
- error: + if ((device = virGetNodeDevice(conn, def->name))) { + if (VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + device = NULL; + } + } + + cleanup: virNodeDeviceObjUnlock(obj); - goto out; + return device; }
-- 2.9.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Move the structures to withing virnodedeviceobj.c Move the typedefs from node_device_conf to virnodedeviceobj.h Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.h | 17 ----------------- src/conf/virnodedeviceobj.c | 11 +++++++++++ src/conf/virnodedeviceobj.h | 6 ++++++ 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 90c7e1f..d10683d 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -328,23 +328,6 @@ struct _virNodeDeviceDef { virNodeDevCapsDefPtr caps; /* optional device capabilities */ }; - -typedef struct _virNodeDeviceObj virNodeDeviceObj; -typedef virNodeDeviceObj *virNodeDeviceObjPtr; -struct _virNodeDeviceObj { - virMutex lock; - - virNodeDeviceDefPtr def; /* device definition */ - -}; - -typedef struct _virNodeDeviceObjList virNodeDeviceObjList; -typedef virNodeDeviceObjList *virNodeDeviceObjListPtr; -struct _virNodeDeviceObjList { - size_t count; - virNodeDeviceObjPtr *objs; -}; - char * virNodeDeviceDefFormat(const virNodeDeviceDef *def); diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index fa61a2d..c19f1e4 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -32,6 +32,17 @@ VIR_LOG_INIT("conf.virnodedeviceobj"); +struct _virNodeDeviceObj { + virMutex lock; + + virNodeDeviceDefPtr def; /* device definition */ +}; + +struct _virNodeDeviceObjList { + size_t count; + virNodeDeviceObjPtr *objs; +}; + static virNodeDeviceObjPtr virNodeDeviceObjNew(virNodeDeviceDefPtr def) diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 6ec5ee7..1122b67 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -27,6 +27,12 @@ # include "object_event.h" +typedef struct _virNodeDeviceObj virNodeDeviceObj; +typedef virNodeDeviceObj *virNodeDeviceObjPtr; + +typedef struct _virNodeDeviceObjList virNodeDeviceObjList; +typedef virNodeDeviceObjList *virNodeDeviceObjListPtr; + typedef struct _virNodeDeviceDriverState virNodeDeviceDriverState; typedef virNodeDeviceDriverState *virNodeDeviceDriverStatePtr; struct _virNodeDeviceDriverState { -- 2.9.4
On Sat, Jun 03, 2017 at 09:12:01AM -0400, John Ferlan wrote:
Move the structures to withing virnodedeviceobj.c
Move the typedefs from node_device_conf to virnodedeviceobj.h
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACK Erik
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock. This also involves creating a virNodeDeviceEndAPI in order to handle the object cleaup for API's that use the Add or Find API's in order to get a locked/reffed object. The EndAPI will unlock and unref the object returning NULL to indicate to the caller to not use the obj. For now this also involves a forward reference to the Unlock, but that'll be removed shortly when the object is convert to use the virObjectLockable shortly. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 156 ++++++++++++++++++----------------- src/conf/virnodedeviceobj.h | 11 +-- src/libvirt_private.syms | 4 +- src/node_device/node_device_driver.c | 17 ++-- src/node_device/node_device_hal.c | 6 +- src/node_device/node_device_udev.c | 12 +-- src/test/test_driver.c | 40 ++++----- 7 files changed, 122 insertions(+), 124 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index c19f1e4..3787b23 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -33,7 +33,7 @@ VIR_LOG_INIT("conf.virnodedeviceobj"); struct _virNodeDeviceObj { - virMutex lock; + virObjectLockable parent; virNodeDeviceDefPtr def; /* device definition */ }; @@ -44,27 +44,63 @@ struct _virNodeDeviceObjList { }; +static virClassPtr virNodeDeviceObjClass; +static void virNodeDeviceObjDispose(void *opaque); + +static int +virNodeDeviceObjOnceInit(void) +{ + if (!(virNodeDeviceObjClass = virClassNew(virClassForObjectLockable(), + "virNodeDeviceObj", + sizeof(virNodeDeviceObj), + virNodeDeviceObjDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNodeDeviceObj) + + +static void +virNodeDeviceObjDispose(void *opaque) +{ + virNodeDeviceObjPtr obj = opaque; + + virNodeDeviceDefFree(obj->def); +} + + static virNodeDeviceObjPtr virNodeDeviceObjNew(virNodeDeviceDefPtr def) { virNodeDeviceObjPtr obj; - if (VIR_ALLOC(obj) < 0) + if (virNodeDeviceObjInitialize() < 0) return NULL; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virObjectLockableNew(virNodeDeviceObjClass))) return NULL; - } - virNodeDeviceObjLock(obj); + + virObjectLock(obj); obj->def = def; return obj; } +void +virNodeDeviceObjEndAPI(virNodeDeviceObjPtr *obj) +{ + if (!*obj) + return; + + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; +} + + virNodeDeviceDefPtr virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj) { @@ -186,13 +222,13 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDeviceDefPtr def; - virNodeDeviceObjLock(obj); + virObjectLock(obj); def = obj->def; if ((def->sysfs_path != NULL) && (STREQ(def->sysfs_path, sysfs_path))) { - return obj; + return virObjectRef(obj); } - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); } return NULL; @@ -209,11 +245,11 @@ virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDeviceDefPtr def; - virNodeDeviceObjLock(obj); + virObjectLock(obj); def = obj->def; if (STREQ(def->name, name)) - return obj; - virNodeDeviceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -231,13 +267,13 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDevCapsDefPtr cap; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if ((cap = virNodeDeviceFindFCCapDef(obj)) && STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) && STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) && virNodeDeviceFindVPORTCapDef(obj)) - return obj; - virNodeDeviceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -254,12 +290,12 @@ virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDevCapsDefPtr cap; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if ((cap = virNodeDeviceFindFCCapDef(obj)) && STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) && virNodeDeviceFindVPORTCapDef(obj)) - return obj; - virNodeDeviceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -275,10 +311,10 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, for (i = 0; i < devs->count; i++) { virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if (virNodeDeviceObjHasCap(obj, cap)) - return obj; - virNodeDeviceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -296,7 +332,7 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDevCapsDefPtr cap; - virNodeDeviceObjLock(obj); + virObjectLock(obj); cap = obj->def->caps; while (cap) { @@ -306,32 +342,18 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { if (STREQ(cap->data.scsi_host.wwnn, wwnn) && STREQ(cap->data.scsi_host.wwpn, wwpn)) - return obj; + return virObjectRef(obj); } } cap = cap->next; } - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); } return NULL; } -void -virNodeDeviceObjFree(virNodeDeviceObjPtr obj) -{ - if (!obj) - return; - - virNodeDeviceDefFree(obj->def); - - virMutexDestroy(&obj->lock); - - VIR_FREE(obj); -} - - virNodeDeviceObjListPtr virNodeDeviceObjListNew(void) { @@ -348,7 +370,7 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) { size_t i; for (i = 0; i < devs->count; i++) - virNodeDeviceObjFree(devs->objs[i]); + virObjectUnref(devs->objs[i]); VIR_FREE(devs->objs); VIR_FREE(devs); } @@ -371,12 +393,11 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) { obj->def = NULL; - virNodeDeviceObjUnlock(obj); - virNodeDeviceObjFree(obj); + virNodeDeviceObjEndAPI(&obj); return NULL; } - return obj; + return virObjectRef(obj); } @@ -386,17 +407,18 @@ virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, { size_t i; - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); + virObjectLock(devs->objs[i]); if (devs->objs[i] == obj) { - virNodeDeviceObjUnlock(devs->objs[i]); + virObjectUnlock(devs->objs[i]); + virObjectUnref(devs->objs[i]); VIR_DELETE_ELEMENT(devs->objs, i, devs->count); break; } - virNodeDeviceObjUnlock(devs->objs[i]); + virObjectUnlock(devs->objs[i]); } } @@ -447,7 +469,7 @@ virNodeDeviceObjListGetParentHostByParent(virNodeDeviceObjListPtr devs, ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -472,7 +494,7 @@ virNodeDeviceObjListGetParentHostByWWNs(virNodeDeviceObjListPtr devs, ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -495,7 +517,7 @@ virNodeDeviceObjListGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -516,7 +538,7 @@ virNodeDeviceObjListFindVportParentHost(virNodeDeviceObjListPtr devs) ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -549,20 +571,6 @@ virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, } -void -virNodeDeviceObjLock(virNodeDeviceObjPtr obj) -{ - virMutexLock(&obj->lock); -} - - -void -virNodeDeviceObjUnlock(virNodeDeviceObjPtr obj) -{ - virMutexUnlock(&obj->lock); -} - - static bool virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, int type) @@ -624,11 +632,11 @@ virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, for (i = 0; i < devs->count; i++) { virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if ((!aclfilter || aclfilter(conn, obj->def)) && (!cap || virNodeDeviceObjHasCap(obj, cap))) ++ndevs; - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); } return ndevs; @@ -648,16 +656,16 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, for (i = 0; i < devs->count && nnames < maxnames; i++) { virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if ((!aclfilter || aclfilter(conn, obj->def)) && (!cap || virNodeDeviceObjHasCap(obj, cap))) { if (VIR_STRDUP(names[nnames], obj->def->name) < 0) { - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); goto failure; } nnames++; } - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); } return nnames; @@ -719,21 +727,21 @@ virNodeDeviceObjListExport(virConnectPtr conn, for (i = 0; i < devs->count; i++) { virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if ((!aclfilter || aclfilter(conn, obj->def)) && virNodeDeviceMatch(obj, flags)) { if (devices) { if (!(device = virGetNodeDevice(conn, obj->def->name)) || VIR_STRDUP(device->parent, obj->def->parent) < 0) { virObjectUnref(device); - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); goto cleanup; } tmp_devices[ndevices] = device; } ndevices++; } - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); } if (tmp_devices) { diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 1122b67..788fb66 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -45,6 +45,8 @@ struct _virNodeDeviceDriverState { virObjectEventStatePtr nodeDeviceEventState; }; +void +virNodeDeviceObjEndAPI(virNodeDeviceObjPtr *obj); virNodeDeviceDefPtr virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj); @@ -76,21 +78,12 @@ virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def, int create); -void -virNodeDeviceObjFree(virNodeDeviceObjPtr dev); - virNodeDeviceObjListPtr virNodeDeviceObjListNew(void); void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs); -void -virNodeDeviceObjLock(virNodeDeviceObjPtr obj); - -void -virNodeDeviceObjUnlock(virNodeDeviceObjPtr obj); - typedef bool (*virNodeDeviceObjListFilter)(virConnectPtr conn, virNodeDeviceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d415888..1eb4502 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -957,7 +957,7 @@ virNetworkObjUpdateAssignDef; # conf/virnodedeviceobj.h -virNodeDeviceObjFree; +virNodeDeviceObjEndAPI; virNodeDeviceObjGetDef; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; @@ -970,8 +970,6 @@ virNodeDeviceObjListGetParentHost; virNodeDeviceObjListNew; virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; -virNodeDeviceObjLock; -virNodeDeviceObjUnlock; # conf/virnwfilterobj.h diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 4a5f168..788b8bc 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -277,7 +277,7 @@ nodeDeviceLookupByName(virConnectPtr conn, } cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return device; } @@ -314,7 +314,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, } cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return device; } @@ -345,7 +345,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device, ret = virNodeDeviceDefFormat(def); cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -373,7 +373,7 @@ nodeDeviceGetParent(virNodeDevicePtr device) } cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -411,7 +411,7 @@ nodeDeviceNumOfCaps(virNodeDevicePtr device) ret = ncaps; cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -460,7 +460,7 @@ nodeDeviceListCaps(virNodeDevicePtr device, ret = ncaps; cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); if (ret == -1) { --ncaps; while (--ncaps >= 0) @@ -613,8 +613,7 @@ nodeDeviceDestroy(virNodeDevicePtr device) * to be taken, so grab the object def which will have the various * fields used to search (name, parent, parent_wwnn, parent_wwpn, * or parent_fabric_wwn) and drop the object lock. */ - virNodeDeviceObjUnlock(obj); - obj = NULL; + virNodeDeviceObjEndAPI(&obj); if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, EXISTING_DEVICE)) < 0) goto cleanup; @@ -626,7 +625,7 @@ nodeDeviceDestroy(virNodeDevicePtr device) cleanup: nodeDeviceUnlock(); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); VIR_FREE(wwnn); VIR_FREE(wwpn); return ret; diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 5521287..d92b47a 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -490,7 +490,7 @@ dev_create(const char *udi) objdef->sysfs_path = devicePath; - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); nodeDeviceUnlock(); return; @@ -545,7 +545,7 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, VIR_DEBUG("%s", name); if (obj) { virNodeDeviceObjListRemove(driver->devs, obj); - virNodeDeviceObjFree(obj); + virObjectUnref(obj); } else { VIR_DEBUG("no device named %s", name); } @@ -568,7 +568,7 @@ device_cap_added(LibHalContext *ctx, if (obj) { def = virNodeDeviceObjGetDef(obj); (void)gather_capability(ctx, udi, cap, &def->caps); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); } else { VIR_DEBUG("no device named %s", name); } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a210fe1..8016d17 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1332,7 +1332,7 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); virNodeDeviceObjListRemove(driver->devs, obj); - virNodeDeviceObjFree(obj); + virObjectUnlock(obj); if (event) virObjectEventStateQueue(driver->nodeDeviceEventState, event); @@ -1369,10 +1369,10 @@ udevSetParent(struct udev_device *device, parent_sysfs_path))) { objdef = virNodeDeviceObjGetDef(obj); if (VIR_STRDUP(def->parent, objdef->name) < 0) { - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); goto cleanup; } - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); if (VIR_STRDUP(def->parent_sysfs_path, parent_sysfs_path) < 0) goto cleanup; @@ -1426,7 +1426,7 @@ udevAddOneDevice(struct udev_device *device) obj = virNodeDeviceObjListFindByName(driver->devs, def->name); if (obj) { - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); new_device = false; } @@ -1443,7 +1443,7 @@ udevAddOneDevice(struct udev_device *device) else event = virNodeDeviceEventUpdateNew(objdef->name); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); ret = 0; @@ -1725,7 +1725,7 @@ udevSetupSystemDev(void) if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); ret = 0; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 67fe252..7b67523 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1175,7 +1175,7 @@ testParseNodedevs(testDriverPtr privconn, goto error; } - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); } ret = 0; @@ -4320,7 +4320,7 @@ testCreateVport(testDriverPtr driver, * create the vHBA. In the long run the result is the same. */ if (!(obj = testNodeDeviceMockCreateVport(driver, wwnn, wwpn))) return -1; - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return 0; } @@ -4532,7 +4532,7 @@ testDestroyVport(testDriverPtr privconn, 0); virNodeDeviceObjListRemove(privconn->devs, obj); - virNodeDeviceObjFree(obj); + virObjectUnref(obj); obj = NULL; testObjectEventQueue(privconn, event); @@ -5334,7 +5334,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name) } } - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -5353,7 +5353,7 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev, ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj)); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -5376,7 +5376,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) "%s", _("no parent for this device")); } - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -5397,7 +5397,7 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev) for (caps = def->caps; caps; caps = caps->next) ++ncaps; - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ncaps; } @@ -5422,13 +5422,13 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) ncaps++; } - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ncaps; error: while (--ncaps >= 0) VIR_FREE(names[ncaps]); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return -1; } @@ -5459,7 +5459,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, goto cleanup; xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy)); - virNodeDeviceObjUnlock(objcopy); + virNodeDeviceObjEndAPI(&objcopy); if (!xml) goto cleanup; @@ -5563,8 +5563,7 @@ testNodeDeviceCreateXML(virConnectPtr conn, dev = NULL; cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testDriverUnlock(driver); virNodeDeviceDefFree(def); virObjectUnref(dev); @@ -5597,28 +5596,29 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) * taken, so we have to dup the parent's name and drop the lock * before calling it. We don't need the reference to the object * any more once we have the parent's name. */ - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); /* We do this just for basic validation, but also avoid finding a * vport capable HBA if for some reason our vHBA doesn't exist */ if (virNodeDeviceObjListGetParentHost(driver->devs, def, - EXISTING_DEVICE) < 0) { - obj = NULL; + EXISTING_DEVICE) < 0) goto cleanup; - } event = virNodeDeviceEventLifecycleNew(dev->name, VIR_NODE_DEVICE_EVENT_DELETED, 0); - virNodeDeviceObjLock(obj); + /* + * + * REVIEW THIS + * + */ virNodeDeviceObjListRemove(driver->devs, obj); - virNodeDeviceObjFree(obj); + virObjectUnref(obj); obj = NULL; cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testObjectEventQueue(driver, event); VIR_FREE(parent_name); VIR_FREE(wwnn); -- 2.9.4
On 06/03/2017 09:12 AM, John Ferlan wrote:
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock.
This also involves creating a virNodeDeviceEndAPI in order to handle the object cleaup for API's that use the Add or Find API's in order to get a locked/reffed object. The EndAPI will unlock and unref the object returning NULL to indicate to the caller to not use the obj.
For now this also involves a forward reference to the Unlock, but that'll be removed shortly when the object is convert to use the virObjectLockable shortly.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 156 ++++++++++++++++++----------------- src/conf/virnodedeviceobj.h | 11 +-- src/libvirt_private.syms | 4 +- src/node_device/node_device_driver.c | 17 ++-- src/node_device/node_device_hal.c | 6 +- src/node_device/node_device_udev.c | 12 +-- src/test/test_driver.c | 40 ++++----- 7 files changed, 122 insertions(+), 124 deletions(-)
[...]
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 67fe252..7b67523 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c
[...] Oh my... looks like I forgot to go back and revisit the following hunk ;-), but of course tripped across it while working through merging changes for patch 1.
@@ -5597,28 +5596,29 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) * taken, so we have to dup the parent's name and drop the lock * before calling it. We don't need the reference to the object * any more once we have the parent's name. */ - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj);
This should be "virObjectUnlock(obj);"
/* We do this just for basic validation, but also avoid finding a * vport capable HBA if for some reason our vHBA doesn't exist */ if (virNodeDeviceObjListGetParentHost(driver->devs, def, - EXISTING_DEVICE) < 0) { - obj = NULL; + EXISTING_DEVICE) < 0)
This would require calling virObjectLock(obj);
goto cleanup; - }
event = virNodeDeviceEventLifecycleNew(dev->name, VIR_NODE_DEVICE_EVENT_DELETED, 0);
- virNodeDeviceObjLock(obj);
and calling virObjectLock(obj) here prior to calling ObjListRemove and setting obj = NULL;
+ /* + * + * REVIEW THIS + * + */ virNodeDeviceObjListRemove(driver->devs, obj); - virNodeDeviceObjFree(obj); + virObjectUnref(obj); obj = NULL;
Hope this makes sense... I'm guessing some of this series will need to be reposted in a v4 anyway - including the "next" logical step to alter the ObjList which is what you're after anyway, but was further down in my original stack of patches. John
cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testObjectEventQueue(driver, event); VIR_FREE(parent_name); VIR_FREE(wwnn);
On Thu, Jun 29, 2017 at 12:11:47PM -0400, John Ferlan wrote:
On 06/03/2017 09:12 AM, John Ferlan wrote:
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock.
This also involves creating a virNodeDeviceEndAPI in order to handle the object cleaup for API's that use the Add or Find API's in order to get a locked/reffed object. The EndAPI will unlock and unref the object returning NULL to indicate to the caller to not use the obj.
For now this also involves a forward reference to the Unlock, but that'll be removed shortly when the object is convert to use the virObjectLockable shortly.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 156 ++++++++++++++++++----------------- src/conf/virnodedeviceobj.h | 11 +-- src/libvirt_private.syms | 4 +- src/node_device/node_device_driver.c | 17 ++-- src/node_device/node_device_hal.c | 6 +- src/node_device/node_device_udev.c | 12 +-- src/test/test_driver.c | 40 ++++----- 7 files changed, 122 insertions(+), 124 deletions(-)
[...]
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 67fe252..7b67523 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c
[...]
Oh my... looks like I forgot to go back and revisit the following hunk ;-), but of course tripped across it while working through merging changes for patch 1.
@@ -5597,28 +5596,29 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) * taken, so we have to dup the parent's name and drop the lock * before calling it. We don't need the reference to the object * any more once we have the parent's name. */ - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj);
This should be "virObjectUnlock(obj);"
/* We do this just for basic validation, but also avoid finding a * vport capable HBA if for some reason our vHBA doesn't exist */ if (virNodeDeviceObjListGetParentHost(driver->devs, def, - EXISTING_DEVICE) < 0) { - obj = NULL; + EXISTING_DEVICE) < 0)
This would require calling virObjectLock(obj);
goto cleanup; - }
event = virNodeDeviceEventLifecycleNew(dev->name, VIR_NODE_DEVICE_EVENT_DELETED, 0);
- virNodeDeviceObjLock(obj);
and calling virObjectLock(obj) here prior to calling ObjListRemove and setting obj = NULL;
+ /* + * + * REVIEW THIS + * + */ virNodeDeviceObjListRemove(driver->devs, obj); - virNodeDeviceObjFree(obj); + virObjectUnref(obj); obj = NULL;
Hope this makes sense... I'm guessing some of this series will need to be reposted in a v4 anyway - including the "next" logical step to alter the ObjList which is what you're after anyway, but was further down in my original stack of patches.
Well, these would be really hard to track, so I agree that posting a v4 would make things easier, since 10 (11 technically) out of 12 patches have already been ACK'd, so it wouldn't prolong the overall process more than just those 2 patches we discuss. Erik
John
cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testObjectEventQueue(driver, event); VIR_FREE(parent_name); VIR_FREE(wwnn);
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ping? Perhaps more involved - the first couple are from the previous series. Once you get past the I hate it when someone renames things - it should be a familiar sequence of creating a lockable object. Tks, John On 06/03/2017 09:11 AM, John Ferlan wrote:
v2: https://www.redhat.com/archives/libvir-list/2017-May/msg00999.html
Some patches from v2 were pushed (3, 4, 6, 7, 8, 9, 11, & 12), but a few remained from that series and are the first 5 patches of this series.
What changed? -> Reworked the virNodeDeviceObjRemove patch (former patch 2, but new series patch 1). That affected the Test patch (former patch 1, but now patch 2). This patch removes the address of obj logic and moves the onus of the ObjFree to the caller (see patch for reason). -> Patch 3 is the former patch 5, with no essential change -> Patch 4 is the former patch 10, with no essential change -> Former patch 13 and 14, were altered to remove the offending address of pointer logic. The result is patch 5 which just essentially former patch 14 without the address of pointer logic.
-> Patches 6-12 for this series are new, but follow along through the logic to make things private.
John Ferlan (12): nodedev: Alter virNodeDeviceObjRemove test: Adjust cleanup/error paths for nodedev test APIs nodedev: Use common naming for virnodedeviceobj nodedev: Use consistent names for driver variables nodedev: Introduce virNodeDeviceObjNew nodedev: Introduce virNodeDeviceObjListNew nodedev: Alter node device obj list function names nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs nodedev: Introduce virNodeDeviceGetSCSIHostCaps nodedev: Introduce virNodeDeviceObjListFindSCSIHostByWWNs nodedev: Privatize _virNodeDeviceObj and _virNodeDeviceObjList nodedev: Convert virNodeDeviceObj to use virObjectLockable
src/conf/node_device_conf.c | 82 ++++++ src/conf/node_device_conf.h | 20 +- src/conf/virnodedeviceobj.c | 420 ++++++++++++++++++------------ src/conf/virnodedeviceobj.h | 67 ++--- src/libvirt_private.syms | 20 +- src/node_device/node_device_driver.c | 159 +++++------ src/node_device/node_device_hal.c | 47 ++-- src/node_device/node_device_linux_sysfs.c | 77 +----- src/node_device/node_device_udev.c | 52 ++-- src/test/test_driver.c | 136 +++++----- 10 files changed, 570 insertions(+), 510 deletions(-)
On Wed, Jun 14, 2017 at 09:27:44PM -0400, John Ferlan wrote:
ping?
Perhaps more involved - the first couple are from the previous series. Once you get past the I hate it when someone renames things - it should be a familiar sequence of creating a lockable object.
Just a small note, as I haven't mentioned anything about that in my reviews so far, we're in a freeze so I'd like you to wait with pushing the patches until we're open again - if that information might have gotten lost in the ton of emails that arrive to the list, so no offense was meant by writing this ;), it's just I've seen a feature (if I remember correctly, definitely not a bugfix) merged withing devel freeze, not that long time ago actually. Erik
On 06/29/2017 10:47 AM, Erik Skultety wrote:
On Wed, Jun 14, 2017 at 09:27:44PM -0400, John Ferlan wrote:
ping?
Perhaps more involved - the first couple are from the previous series. Once you get past the I hate it when someone renames things - it should be a familiar sequence of creating a lockable object.
Just a small note, as I haven't mentioned anything about that in my reviews so far, we're in a freeze so I'd like you to wait with pushing the patches until we're open again - if that information might have gotten lost in the ton of emails that arrive to the list, so no offense was meant by writing this ;), it's just I've seen a feature (if I remember correctly, definitely not a bugfix) merged withing devel freeze, not that long time ago actually.
Erik
No worries - I'd wait for the next release anyway, much safer that way. Still great to get a review now in order to be ready once the race to commit is back on ;-) John
participants (3)
-
Daniel P. Berrange -
Erik Skultety -
John Ferlan