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

Erik Skultety eskultet at redhat.com
Tue Nov 14 14:17:44 UTC 2017


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




More information about the libvir-list mailing list