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

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