On Wed, Nov 15, 2017 at 05:59:39PM -0500, John Ferlan wrote: > > > On 11/14/2017 09:17 AM, Erik Skultety wrote: > > On Sun, Nov 12, 2017 at 09:46:48AM -0500, John Ferlan wrote: > >> > >> [...] > >> > >>> Now the actual review. > >>> virNetDaemonAddServer is only used when spawning a new daemon or setting up LXC > >>> controller. The function essentially does: > >>> > >>> lock(@dmn) > >>> hash_table_add(@srv) > >>> ref(@srv) > >>> unlock(@dmn) > >>> > >>> and then you unref @dmn right upon the completion of adding the server to the > >>> hash table, what's the purpose of the extra ref when you discard it > >>> immediately? Unless I'm failing to see something, I'd prefer to just drop the > >>> extra ref in virNetDaemonAddServer and leave the proposed hunks unchanged - you > >>> have 1 ref which you got by creating the object, now it should be just simply > >>> inserted into the hash table, the additional ref after insertion doesn't make > >>> sense, if someone managed to unref it before inserting it into the hash table, > >>> hash insert would most likely fail, if not, hashFree surely would, not > >>> mentioning that at that point there's no entity that is touching servers. > >>> > >> > >> Since I was "digging" on a different issue - check out how this is done > >> elsewhere... Start with virNetServerDispatchNewClient. It'll call the > >> ClientNew function which generates a REF. Then it will call AddClient > >> which generates a REF. Then because it's on that list and this code is > >> theoretically done with it - it will UNREF the client before returning. > > > > Sure, but we shouldn't treat everything in a uniform manner - the fact that we > > can do that and it works still doesn't necessarily mean it's right. BTW I only > > looked at NetClient briefly, but that too looks to me like the reffing is > > unnecessary. > > > > Erik > > > > I do understand your point, but undoing the Ref when placing into the > HashTable consistently across all consumers is perhaps a large task. > > At this point, I'm thinking I just drop patches 3 & 4 - it just means > the objects will have 2 refs and allows me/us to just make forward > progress putting the discussion about the need to Ref when adding to the OK, we can do that... > hash table for a different day or set of patches. In the end the more > important patch is 5 - at least with respect to does it handle the issue > that Nikolay was originally trying to handle. Damn, I forgot to respond to patch 5, sorry about that, I just did. Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
