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