[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]