8:11 a.m.
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
8:11 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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(a)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
7:12 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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
7:24 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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 :|
8:19 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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
8:57 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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
9:28 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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
11:07 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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
3:40 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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.
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.
[1] https://www.redhat.com/archives/libvir-list/2017-June/msg01328.html
6:14 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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
6:41 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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
8:47 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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
10:20 a.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:41 p.m.
New subject: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
[...]
>
> 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
8:11 a.m.
New subject: [libvirt] [PATCH v3 02/12] test: Adjust cleanup/error paths for nodedev test APIs
- 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(a)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
7:57 a.m.
New subject: [libvirt] [PATCH v3 02/12] test: Adjust cleanup/error paths for nodedev test APIs
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(a)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
9:09 a.m.
New subject: [libvirt] [PATCH v3 02/12] test: Adjust cleanup/error paths for nodedev test APIs
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(a)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
9:40 a.m.
New subject: [libvirt] [PATCH v3 02/12] test: Adjust cleanup/error paths for nodedev test APIs
> 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
8:11 a.m.
New subject: [libvirt] [PATCH v3 03/12] nodedev: Use common naming for virnodedeviceobj
A virNodeDeviceObjPtr is an @obj
A virNodeDeviceObjListPtr is an @devs
Signed-off-by: John Ferlan <jferlan(a)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
8:49 a.m.
New subject: [libvirt] [PATCH v3 03/12] nodedev: Use common naming for virnodedeviceobj
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(a)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
8:11 a.m.
New subject: [libvirt] [PATCH v3 04/12] nodedev: Use consistent names for driver variables
A virNodeDeviceObjPtr is an @obj
A virNodeDeviceObjListPtr is a @devs
A virNodeDevicePtr is a @device
Signed-off-by: John Ferlan <jferlan(a)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
9:06 a.m.
New subject: [libvirt] [PATCH v3 04/12] nodedev: Use consistent names for driver variables
[...]
@@ -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
8:11 a.m.
New subject: [libvirt] [PATCH v3 05/12] nodedev: Introduce virNodeDeviceObjNew
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(a)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
9:41 a.m.
New subject: [libvirt] [PATCH v3 05/12] nodedev: Introduce virNodeDeviceObjNew
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(a)redhat.com>
---
ACK
Erik
8:11 a.m.
New subject: [libvirt] [PATCH v3 06/12] nodedev: Introduce virNodeDeviceObjListNew
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(a)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
5:29 a.m.
New subject: [libvirt] [PATCH v3 06/12] nodedev: Introduce virNodeDeviceObjListNew
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(a)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
8:11 a.m.
New subject: [libvirt] [PATCH v3 07/12] nodedev: Alter node device obj list function names
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(a)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
6:30 a.m.
New subject: [libvirt] [PATCH v3 07/12] nodedev: Alter node device obj list function names
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(a)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.
8:11 a.m.
New subject: [libvirt] [PATCH v3 08/12] nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs
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(a)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
7:06 a.m.
New subject: [libvirt] [PATCH v3 08/12] nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs
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(a)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
12:08 p.m.
New subject: [libvirt] [PATCH v3 08/12] nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs
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(a)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
2:13 a.m.
New subject: [libvirt] [PATCH v3 08/12] nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs
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(a)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
8:11 a.m.
New subject: [libvirt] [PATCH v3 09/12] nodedev: Introduce virNodeDeviceGetSCSIHostCaps
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(a)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
8:12 a.m.
New subject: [libvirt] [PATCH v3 09/12] nodedev: Introduce virNodeDeviceGetSCSIHostCaps
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
8:12 a.m.
New subject: [libvirt] [PATCH v3 10/12] nodedev: Introduce virNodeDeviceObjListFindSCSIHostByWWNs
Alter the nodeDeviceLookupSCSIHostByWWN to use the new API in order
to find what it's looking for.
Signed-off-by: John Ferlan <jferlan(a)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
8:12 a.m.
New subject: [libvirt] [PATCH v3 10/12] nodedev: Introduce virNodeDeviceObjListFindSCSIHostByWWNs
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:54 p.m.
New subject: [libvirt] [PATCH v3 10/12] nodedev: Introduce virNodeDeviceObjListFindSCSIHostByWWNs
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(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
8:12 a.m.
New subject: [libvirt] [PATCH v3 11/12] nodedev: Privatize _virNodeDeviceObj and _virNodeDeviceObjList
Move the structures to withing virnodedeviceobj.c
Move the typedefs from node_device_conf to virnodedeviceobj.h
Signed-off-by: John Ferlan <jferlan(a)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
7:35 a.m.
New subject: [libvirt] [PATCH v3 11/12] nodedev: Privatize _virNodeDeviceObj and _virNodeDeviceObjList
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(a)redhat.com>
ACK
Erik
8:12 a.m.
New subject: [libvirt] [PATCH v3 12/12] nodedev: Convert virNodeDeviceObj to use virObjectLockable
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(a)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
11:11 a.m.
New subject: [libvirt] [PATCH v3 12/12] nodedev: Convert virNodeDeviceObj to use virObjectLockable
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(a)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);
10:22 a.m.
New subject: [libvirt] [PATCH v3 12/12] nodedev: Convert virNodeDeviceObj to use virObjectLockable
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:27 p.m.
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(-)
9:47 a.m.
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
11:13 a.m.
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
2701
days inactive
2731
days old
44 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Erik Skultety -
John Ferlan