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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
