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


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