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
>
>From 6f432306d033cd9bf3494f3f39e4369ead3e5206 Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan@xxxxxxxxxx>
Date: Thu, 29 Jun 2017 11:28:53 -0400
Subject: [PATCH 2/2] merge and alter commit message to
Rather than passing the object to be removed by reference, pass by value
and force the caller to choose to either not use the object again or to
set the object value to NULL in order to avoid it being used. This undoes
logic from commit id '611480747' which changed to pass by reference, set
@dev = NULL. The replacement is to have the calling function set what was
removed to NULL in order to avoid subsequent virNodeDeviceObjUnlock call.
Additionally, modify the dev_refresh code in node_device_hal to use a
new bool @rediscover rather than referencing @dev after the call (to
avoid any future attempt to dereference).
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
src/conf/virnodedeviceobj.c | 1 +
src/node_device/node_device_hal.c | 10 +++++-----
src/node_device/node_device_udev.c | 1 -
src/test/test_driver.c | 2 --
4 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index fa73de1..c6caa68 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -311,6 +311,7 @@ virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
virNodeDeviceObjLock(devs->objs[i]);
if (devs->objs[i] == dev) {
virNodeDeviceObjUnlock(devs->objs[i]);
+ virNodeDeviceObjFree(devs->objs[i]);
VIR_DELETE_ELEMENT(devs->objs, i, devs->count);
break;
diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
index c354cd3..0346d5a 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -507,6 +507,7 @@ dev_refresh(const char *udi)
{
const char *name = hal_name(udi);
virNodeDeviceObjPtr dev;
+ bool rediscover = false;
nodeDeviceLock();
dev = virNodeDeviceObjFindByName(&driver->devs, name);
@@ -515,12 +516,13 @@ dev_refresh(const char *udi)
* to sub-capabilities (like net.80203) is nasty ... so avoid it.
*/
virNodeDeviceObjRemove(&driver->devs, dev);
+ rediscover = true;
} else {
VIR_DEBUG("no device named %s", name);
}
nodeDeviceUnlock();
- if (dev)
+ if (rediscover)
dev_create(udi);
}
@@ -543,12 +545,10 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED,
nodeDeviceLock();
dev = virNodeDeviceObjFindByName(&driver->devs, name);
VIR_DEBUG("%s", name);
- if (dev) {
+ if (dev)
virNodeDeviceObjRemove(&driver->devs, dev);
- virNodeDeviceObjFree(dev);
- } else {
+ 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 819e4e7..a0dbb2a 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1334,7 +1334,6 @@ udevRemoveOneDevice(struct udev_device *device)
VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
def->name, name);
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 04887e0..f3773d3 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4531,7 +4531,6 @@ testDestroyVport(testDriverPtr privconn,
0);
virNodeDeviceObjRemove(&privconn->devs, obj);
- virNodeDeviceObjFree(obj);
obj = NULL;
ret = 0;
@@ -5627,7 +5626,6 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
virNodeDeviceObjLock(obj);
virNodeDeviceObjRemove(&driver->devs, obj);
- virNodeDeviceObjFree(obj);
obj = NULL;
out:
--
2.9.4
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
