Re: [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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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
Re: [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 >>>
Re: [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 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
Re: [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 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
Re: [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 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
Re: [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 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 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(>devs, ); +virNodeDeviceObjRemove(>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 >>>
Re: [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(>devs, ); > >> +virNodeDeviceObjRemove(>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 loveand 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
Re: [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(>devs, ); >> +virNodeDeviceObjRemove(>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
Re: [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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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(>devs, ); > > +virNodeDeviceObjRemove(>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
Re: [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(>devs, ); > +virNodeDeviceObjRemove(>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
