Rather than passing the object to be removed by reference, pass by value
and then let the caller decide whether or not the object should be free'd.
This function should just handle the remove of the object from the list
for which it was placed during virNodeDeviceObjAssignDef.

One caller in node_device_hal would fail to go through the dev_create path
since the @dev would have been NULL after returning from the Remove API.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/virnodedeviceobj.c        | 14 ++++++--------
 src/conf/virnodedeviceobj.h        |  2 +-
 src/libvirt_private.syms           |  1 +
 src/node_device/node_device_hal.c  | 10 ++++++----
 src/node_device/node_device_udev.c |  3 ++-
 src/test/test_driver.c             |  8 ++++++--
 6 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index e78f451..fa73de1 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -301,23 +301,21 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
 
 void
 virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
-                       virNodeDeviceObjPtr *dev)
+                       virNodeDeviceObjPtr dev)
 {
     size_t i;
 
-    virNodeDeviceObjUnlock(*dev);
+    virNodeDeviceObjUnlock(dev);
 
     for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjLock(*dev);
-        if (devs->objs[i] == *dev) {
-            virNodeDeviceObjUnlock(*dev);
-            virNodeDeviceObjFree(devs->objs[i]);
-            *dev = NULL;
+        virNodeDeviceObjLock(devs->objs[i]);
+        if (devs->objs[i] == dev) {
+            virNodeDeviceObjUnlock(devs->objs[i]);
 
             VIR_DELETE_ELEMENT(devs->objs, i, devs->count);
             break;
         }
-        virNodeDeviceObjUnlock(*dev);
+        virNodeDeviceObjUnlock(devs->objs[i]);
     }
 }
 
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index 05a9d11..9bc02ee 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -58,7 +58,7 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
 
 void
 virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
-                       virNodeDeviceObjPtr *dev);
+                       virNodeDeviceObjPtr dev);
 
 int
 virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 683a232..4a10508 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -959,6 +959,7 @@ virNetworkObjUpdateAssignDef;
 virNodeDeviceObjAssignDef;
 virNodeDeviceObjFindByName;
 virNodeDeviceObjFindBySysfsPath;
+virNodeDeviceObjFree;
 virNodeDeviceObjGetDef;
 virNodeDeviceObjGetNames;
 virNodeDeviceObjGetParentHost;
diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
index f468e42..c354cd3 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -514,7 +514,7 @@ dev_refresh(const char *udi)
         /* Simply "rediscover" device -- incrementally handling changes
          * to sub-capabilities (like net.80203) is nasty ... so avoid it.
          */
-        virNodeDeviceObjRemove(&driver->devs, &dev);
+        virNodeDeviceObjRemove(&driver->devs, dev);
     } else {
         VIR_DEBUG("no device named %s", name);
     }
@@ -543,10 +543,12 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED,
     nodeDeviceLock();
     dev = virNodeDeviceObjFindByName(&driver->devs, name);
     VIR_DEBUG("%s", name);
-    if (dev)
-        virNodeDeviceObjRemove(&driver->devs, &dev);
-    else
+    if (dev) {
+        virNodeDeviceObjRemove(&driver->devs, dev);
+        virNodeDeviceObjFree(dev);
+    } else {
         VIR_DEBUG("no device named %s", name);
+    }
     nodeDeviceUnlock();
 }
 
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 174124a..819e4e7 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1333,7 +1333,8 @@ udevRemoveOneDevice(struct udev_device *device)
 
     VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
               def->name, name);
-    virNodeDeviceObjRemove(&driver->devs, &dev);
+    virNodeDeviceObjRemove(&driver->devs, dev);
+    virNodeDeviceObjFree(dev);
 
     if (event)
         virObjectEventStateQueue(driver->nodeDeviceEventState, event);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index e5938f5..e323619 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4530,7 +4530,9 @@ testDestroyVport(testDriverPtr privconn,
                                            VIR_NODE_DEVICE_EVENT_DELETED,
                                            0);
 
-    virNodeDeviceObjRemove(&privconn->devs, &obj);
+    virNodeDeviceObjRemove(&privconn->devs, obj);
+    virNodeDeviceObjFree(obj);
+    obj = NULL;
 
     ret = 0;
 
@@ -5624,7 +5626,9 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
                                            0);
 
     virNodeDeviceObjLock(obj);
-    virNodeDeviceObjRemove(&driver->devs, &obj);
+    virNodeDeviceObjRemove(&driver->devs, obj);
+    virNodeDeviceObjFree(obj);
+    obj = NULL;
 
  out:
     if (obj)
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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
> 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 06/30/2017 04:40 AM, Erik Skultety wrote:
> On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote:
>>
>>
>> On 06/29/2017 10:28 AM, Erik Skultety wrote:
>>> On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote:
>>>>
>>>>
>>>> On 06/29/2017 08:12 AM, Erik Skultety wrote:
>>>>> On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
>>>>>> Rather than passing the object to be removed by reference, pass by value
>>>>>> and then let the caller decide whether or not the object should be free'd.
>>>>>> This function should just handle the remove of the object from the list
>>>>>> for which it was placed during virNodeDeviceObjAssignDef.
>>>>>>
>>>>>> One caller in node_device_hal would fail to go through the dev_create path
>>>>>> since the @dev would have been NULL after returning from the Remove API.
>>>>>
>>>>> This is the main motivation for the patch I presume - in which case, I'm
>>>>> wondering why do we actually have to remove the device from the list when
>>>>> handling 'change'/'update' for hal instead of just replacing the ->def with a
>>>>> new instance but it's perfectly fine to do that for udev...I don't see the
>>>>> point in doing what we currently do for hal.
>>>>>
>>>>> [...]
>>>>
>>>> The main motivation is that in the previous review pass there was a
>>>> "dislike" of passing the pointer to a pointer for something else I
>>>> changed, see:
>>>>
>>>> https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html
>>>>
>>>> Also the initial pass at altering this function was questioned, see:
>>>>
>>>> https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html
>>>>
>>>
>>> Well, that comment is true, but the commit message of this patch says that it
>>> drops the requirement of passing by reference, thus leaving the responsibility
>>> to free the obj to the caller. Now, the way I see it what we should aim at
>>> achieving here is reference counted objects, so the vir*ObjFree in the caller
>>> would become and *Unref. Therefore IMHO it's not the best approach to introduce
>>> another boolean for HAL and leave the vir*ObjFree in the Remove routine, you
>>> wouldn't be clearing the object for the caller anyway, so I don't think that is
>>> the way to go.
>>>
>>
>> I actually think the better course of action is to be more like the
>> other functions. I believe virNodeDeviceObjRemove should do the
>> virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the
>> alloc and list insertion and *Remove is the antecedent doing the list
> 
> Well, eventually we'll hopefully end up with reference-counted objects so doing
> it with having *ObjFree in ObjRemove or in the caller won't matter in the end.

Keeping the Free/Unref as part of the callers responsibility would mean
that once we ended up with common object code doing the remove, then
each of the nodedev callers would need to remove their extra Free/Unref.
Which like you point out below is essentially equal work as long as one
remembers that when it comes time ;-).

> From my perspective, if it's the caller who's responsible to free it, they'll
> know they cannot touch the pointer afterwards (yeah...I assumed there are no
> bugs) whereas if freeing the object is done as part of ObjRemove, it's the
> caller who's the one to assume if the object was both freed and cleared or just
> freed or neither - which I don't like that much, but again, matter of
> perspective, I see your reasoning though.
> You'll have to replace frees with unrefs anyway, thus you won't make it any
> easier by this approach, the amount of work in both cases we're discussing here
> is equal, so I'll be probably fine with that adjustment as well.
> 
>> removal and free. I will adjust the commit message and can even add
>> comments prior to the function (if desired; however, it'll eventually be
>> just a wrapper to a more generic object function).
>>
>> Also, light has dawned over marble head and for hal's dev_refresh the
>> logic will then work correctly anyway since &dev wouldn't be set to
>> NULL. The code doesn't reference data in @dev. All that is important is
> 
> Relying on a dangling pointer just for the sole purpose of checking 'yep, there
> used to be something some time ago' would be very very very poor and fragile
> design. Adding a bool there is still only a workaround of a previous workaround
> (the commit you referenced), which doesn't necessarily make the 'more nicely
> packaged' fix a better solution. Per [1] we should call dev_create(udi) right
> away, dropping the driver lock just before the call - so much more readable.
> So I don't agree with the boolean part of the patch attached.

Personally I was more in favor of the pass by reference logic because it
leaves no question. I'm not as much of a fan of the (sometimes hidden)
knowledge that a function "consumes" a callers pointer. We have quite a
few of those sprinkled throughout the code. The few code analyzers I've
used over time prefer logic that doesn't require some caller to set a
pointer they passed to a function to NULL after the function
successfully completes. Opinions in this team are different though.

Anyway, from your comment to drop the boolean the logic is either:

Lock
dev = FindByName
if (dev)
   Remove
   Unlock
   dev_create
else
   DEBUG
   Unlock

Or adding a return after the dev_create to avoid having a second Unlock
in the else.  IDC either way, just want to be clear.

FWIW: The boolean would be short lived... Once the ObjList is a self
locking virLockableObject (patch 13), the boolean would disappear in
patch 14 with the dev_create moving inside the "if" part. I already have
that in my branch, but I'm sure it wouldn't apply due to merge conflicts
as a result of changes made as part of the review process.

John

> 
> [1] https://www.redhat.com/archives/libvir-list/2017-June/msg01328.html
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Jun 30, 2017 at 09:47:07AM -0400, John Ferlan wrote:
>
>
> On 06/30/2017 07:41 AM, Erik Skultety wrote:
> > On Fri, Jun 30, 2017 at 07:14:13AM -0400, John Ferlan wrote:
> >>
> >>
> >> On 06/30/2017 04:40 AM, Erik Skultety wrote:
> >>> On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote:
> >>>>
> >>>>
> >>>> On 06/29/2017 10:28 AM, Erik Skultety wrote:
> >>>>> On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 06/29/2017 08:12 AM, Erik Skultety wrote:
> >>>>>>> On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
> >>>>>>>> Rather than passing the object to be removed by reference, pass by value
> >>>>>>>> and then let the caller decide whether or not the object should be free'd.
> >>>>>>>> This function should just handle the remove of the object from the list
> >>>>>>>> for which it was placed during virNodeDeviceObjAssignDef.
> >>>>>>>>
> >>>>>>>> One caller in node_device_hal would fail to go through the dev_create path
> >>>>>>>> since the @dev would have been NULL after returning from the Remove API.
> >>>>>>>
> >>>>>>> This is the main motivation for the patch I presume - in which case, I'm
> >>>>>>> wondering why do we actually have to remove the device from the list when
> >>>>>>> handling 'change'/'update' for hal instead of just replacing the ->def with a
> >>>>>>> new instance but it's perfectly fine to do that for udev...I don't see the
> >>>>>>> point in doing what we currently do for hal.
> >>>>>>>
> >>>>>>> [...]
> >>>>>>
> >>>>>> The main motivation is that in the previous review pass there was a
> >>>>>> "dislike" of passing the pointer to a pointer for something else I
> >>>>>> changed, see:
> >>>>>>
> >>>>>> https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html
> >>>>>>
> >>>>>> Also the initial pass at altering this function was questioned, see:
> >>>>>>
> >>>>>> https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html
> >>>>>>
> >>>>>
> >>>>> Well, that comment is true, but the commit message of this patch says that it
> >>>>> drops the requirement of passing by reference, thus leaving the responsibility
> >>>>> to free the obj to the caller. Now, the way I see it what we should aim at
> >>>>> achieving here is reference counted objects, so the vir*ObjFree in the caller
> >>>>> would become and *Unref. Therefore IMHO it's not the best approach to introduce
> >>>>> another boolean for HAL and leave the vir*ObjFree in the Remove routine, you
> >>>>> wouldn't be clearing the object for the caller anyway, so I don't think that is
> >>>>> the way to go.
> >>>>>
> >>>>
> >>>> I actually think the better course of action is to be more like the
> >>>> other functions. I believe virNodeDeviceObjRemove should do the
> >>>> virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the
> >>>> alloc and list insertion and *Remove is the antecedent doing the list
> >>>
> >>> Well, eventually we'll hopefully end up with reference-counted objects so doing
> >>> it with having *ObjFree in ObjRemove or in the caller won't matter in the end.
> >>
> >> Keeping the Free/Unref as part of the callers responsibility would mean
> >> that once we ended up with common object code doing the remove, then
> >> each of the nodedev callers would need to remove their extra Free/Unref.
> >
> > I don't think so. Looking at other examples, what happens in most cases is that
> > caller already gets the ref'd object, calls Remove() which calls Unref(),
> > returns to the caller who then calls EndAPI() which in turn destroys the last
> > reference. Therefore in here, to actually get rid of the object all the callers
> > would need to do essentially the same thing - keep their Unref() in the function
> > block, solely because GetObj() returns an already locked and ref'd object.
> >
>
> Doh...  true - of course I have in mind the end goal which is making
> this whole process awfully difficult. Going step by step hasn't really
> helped as much as it should have /-(.  Adjustments to later patches will
> be necessary to either Unref or EndAPI.

I'm sorry, I don't follow. What changes to later patches are we talking about
exactly? I agree with the original approach you took because IMHO it is the most
transparent in terms of gradual changes - replacing frees with unrefs or EndAPIs
for that matter, and it also keeps things consistent.

>
> Without getting into specifics of each EndAPI model (domain, network,
> and secret) - I'm assuming you can agree that each does things a bit
> differently and gives a sense for the impetus behind this overall
> effort. I generally sit here looking at code keeping track on my fingers
> of the RefCnt and whether the lock is held ;-)
>
> >> Which like you point out below is essentially equal work as long as one
> >> remembers that when it comes time ;-).
> >>
> >>> From my perspective, if it's the caller who's responsible to free it, they'll
> >>> know they cannot touch the pointer afterwards (yeah...I assumed there are no
> >>> bugs) whereas if freeing the object is done as part of ObjRemove, it's the
> >>> caller who's the one to assume if the object was both freed and cleared or just
> >>> freed or neither - which I don't like that much, but again, matter of
> >>> perspective, I see your reasoning though.
> >>> You'll have to replace frees with unrefs anyway, thus you won't make it any
> >>> easier by this approach, the amount of work in both cases we're discussing here
> >>> is equal, so I'll be probably fine with that adjustment as well.
> >>>
> >>>> removal and free. I will adjust the commit message and can even add
> >>>> comments prior to the function (if desired; however, it'll eventually be
> >>>> just a wrapper to a more generic object function).
> >>>>
> >>>> Also, light has dawned over marble head and for hal's dev_refresh the
> >>>> logic will then work correctly anyway since &dev wouldn't be set to
> >>>> NULL. The code doesn't reference data in @dev. All that is important is
> >>>
> >>> Relying on a dangling pointer just for the sole purpose of checking 'yep, there
> >>> used to be something some time ago' would be very very very poor and fragile
> >>> design. Adding a bool there is still only a workaround of a previous workaround
> >>> (the commit you referenced), which doesn't necessarily make the 'more nicely
> >>> packaged' fix a better solution. Per [1] we should call dev_create(udi) right
> >>> away, dropping the driver lock just before the call - so much more readable.
> >>> So I don't agree with the boolean part of the patch attached.
> >>
> >> Personally I was more in favor of the pass by reference logic because it
> >> leaves no question. I'm not as much of a fan of the (sometimes hidden)
> >> knowledge that a function "consumes" a callers pointer. We have quite a
> >> few of those sprinkled throughout the code. The few code analyzers I've
> >> used over time prefer logic that doesn't require some caller to set a
> >> pointer they passed to a function to NULL after the function
> >> successfully completes. Opinions in this team are different though.
> >>
> >> Anyway, from your comment to drop the boolean the logic is either:
> >>
> >> Lock
> >> dev = FindByName
> >> if (dev)
> >>    Remove
> >
> > I wanted to get rid of this^ completely....in a separate patch...
> >
> >>    Unlock
> >>    dev_create
> >
> > and call ^this directly, the remove there doesn't serve a purpose anymore.
> >
>
> I'm not sure I follow... Like noted previously trying not to disturb the
> logic here - just applying a different band-aid
>
> The purpose of the Remove AIUI is to allow the code to rediscover some
> device because no one wanted to handle the messy situation of some
> capability being removed or some property being modified. Instead it was
> deemed a better option to just Remove the device and allow dev_create
> reinstantiate.

It's been a long time since 2008 and there wasn't a exhausting description why
it was a problem back in 2008 - locks maybe?. Commits d48717054c7 and 6244c173b
don't really relate to what I'm suggesting, do they? You wouldn't mess around
with locks, so you wouldn't regress with deadlock. I'm still trying to figure
out what I'm missing here, since you're holding all the necessary locks anyway
so what could possibly be the benefit of calling remove on the device prior to
creating-refreshing it next.

Erik

>
> Go back to the original implementation, commit id '620d4be7a'... The
> locks were added as part of commit id 'd48717054c7' and the dev_refresh
> in commit id '6244c173b' (to fix a deadlock).
>
> I think the Remove is necessary.
>
> >> else
> >>    DEBUG
> >>    Unlock
> >>
> >> Or adding a return after the dev_create to avoid having a second Unlock
> >> in the else.  IDC either way, just want to be clear.
> >>
> >> FWIW: The boolean would be short lived... Once the ObjList is a self
> >> locking virLockableObject (patch 13), the boolean would disappear in
> >
> > I know it would - so why introduce it in the first place if we can achieve the
> > same thing in a different way, especially because its removal would be only a
> > couple of patches apart (I know...there are exceptions - this does not qualify
> > as one).
> >
> > Erik
> >
>
> I can remove the bool, IDC, but unless I can understand why you don't
> think the Remove is necessary - it's either a bool or a move
> dev_create() inside the if, after an Unlock and a return in the if
> condition (at least temporarily).
>
> John
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

>>
>> Doh...  true - of course I have in mind the end goal which is making
>> this whole process awfully difficult. Going step by step hasn't really
>> helped as much as it should have /-(.  Adjustments to later patches will
>> be necessary to either Unref or EndAPI.
> 
> I'm sorry, I don't follow. What changes to later patches are we talking about
> exactly? I agree with the original approach you took because IMHO it is the most
> transparent in terms of gradual changes - replacing frees with unrefs or EndAPIs
> for that matter, and it also keeps things consistent.
> 

Pay no attention to the old man talking to himself ;-)

I've gone back to the original 1/12 and just added a
virNodeDeviceObjFree within the if (dev) prior to dev_create(udi). Since
it doesn't matter if the node device is unlocked it's a safe place to
put it.

[...]

>>
>> I'm not sure I follow... Like noted previously trying not to disturb the
>> logic here - just applying a different band-aid
>>
>> The purpose of the Remove AIUI is to allow the code to rediscover some
>> device because no one wanted to handle the messy situation of some
>> capability being removed or some property being modified. Instead it was
>> deemed a better option to just Remove the device and allow dev_create
>> reinstantiate.
> 
> It's been a long time since 2008 and there wasn't a exhausting description why
> it was a problem back in 2008 - locks maybe?. Commits d48717054c7 and 6244c173b
> don't really relate to what I'm suggesting, do they? You wouldn't mess around
> with locks, so you wouldn't regress with deadlock. I'm still trying to figure
> out what I'm missing here, since you're holding all the necessary locks anyway
> so what could possibly be the benefit of calling remove on the device prior to
> creating-refreshing it next.
> 
> Erik
> 

I was trying to figure out why you were advocating removing the Find and
Remove and just call dev_create directly.

AIUI, the code is removing the device and allowing dev_create recreate
it because it was easier than deal with def->caps replacement. Sure
maybe that's easier today, but it's been way too long for me to recall
much about hal other than something to do with 2001: A Space Odyssey
("I'm sorry Dave, I'm afraid I can't do that")...

John

v4 will be posted soon

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list