Re: [libvirt] [PATCH 3/5] libvirtd: Alter refcnt processing for domain server objects

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

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.