8:14 a.m.
On Fri, Nov 10, 2017 at 05:41:51PM -0500, John Ferlan wrote:
On 11/10/2017 10:08 AM, Erik Skultety wrote:
> On Tue, Nov 07, 2017 at 09:39:54PM -0500, John Ferlan wrote:
>> Whether the @srv/@srvAdm is added to the dmn->servers list or not,
>> the reference kept for the allocation can be dropped leaving just the
>> reference for being on the dmn->servers list be the sole deciding
>> factor when to really free the associated memory. The @dmn dispose
>> function (virNetDaemonDispose) will handle the Unref of the objects
>> during the virHashFree.
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> daemon/libvirtd.c | 15 +++++++++++----
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
>> index 73f24915df..5c47e49d48 100644
>> --- a/daemon/libvirtd.c
>> +++ b/daemon/libvirtd.c
>> @@ -1065,6 +1065,7 @@ int main(int argc, char **argv) {
>> char *remote_config_file = NULL;
>> int statuswrite = -1;
>> int ret = 1;
>> + int rc;
>> int pid_file_fd = -1;
>> char *pid_file = NULL;
>> char *sock_file = NULL;
>> @@ -1319,7 +1320,11 @@ int main(int argc, char **argv) {
>> goto cleanup;
>> }
>>
>> - if (virNetDaemonAddServer(dmn, srv) < 0) {
>> + /* Add @srv to @dmn servers hash table and Unref to allow removal from
>> + * hash table at @dmn disposal to decide when to free @srv */
>> + rc = virNetDaemonAddServer(dmn, srv);
>
> <rant>
> Sorry for the delay, I've been playing with LXC for 2 days, trying to either
> debug or just use valgrind on lxc_controller to get the info I need
> (explanation below), but after those 2 days, I just give up, not worth any more
> time, if someone knows how I can actually hit the cleanup section in
> lxc_controller within gdb telling me that it suddenly temporarily disabled
> breakpoints at the moment it's supposed to stop at one that it had let me to
set
> earlier, I'm all ears. Btw to my biggest surprise valgrind still doesn't
handle
> setns syscall which has probably been the reason why we forbade running LXC
> under valgrind in 2009, unbelievable. </rant>
I feel your pain ;-) Chasing these memory things is challenging
especially when variable names change or things just aren't consistent.
Well, it wouldn't have to be if one didn't have to fight the mentioned tools so
much - if you've been using fedora since v25 (I don't know whether debian/ubuntu
suffer the same way), you might have come across this weird thing that even if
you compile libvirt with debugging symbols, you're not able to actually step
into non-static functions with "step" (you'd have to use stepi multiple
times)
because the linker doesn't generate the PLT stubs for some reason, which is
quite frustrating for someone who debugs a lot and more breakpoints just don't
help here really since some of them might get hit constantly from places I don't
want and disabling them simply doesn't feel efficient like using 'step'.
...
Anyway, I agree with you upon visual inspection - there's a leak
since
@srv is never Unref'd. Once it's added to the Server hash table, then
there should be a virObjectUnref(srv).
\o/ This is just one in a ... lot ..., since the Unrefs are almost impossible
to track visually... :)
As for other consumers.... The reason that log_daemon doesn't do the
virObjectUnref is that it saves @srv and eventually will Unref it later.
Unlike the lock_daemon code which uses virNetDaemonGetServer to find the
@srv; whereas, log_daemon goes direct to logDaemon->srv.
Yeah, that just doesn't feel right, virtlogd is just a copy of virtlockd, I
haven't found a compelling reason why they're different in this aspect, so I
sent simple patches to address that.
>
> 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?
The REF is for the Hash Table. We've added something to the hash table -
we should increment the refcnt - nothing more nothing less. The UNREF
for that occurs as part of virHashFree because virHashCreate uses
virObjectFreeHashData.
Sure, I understand that the ref is for the hash table, but there's no other
caller that would interfere with the object (the idea behind ref counting), you
already have a ref for the daemon, which is not going to need it anymore (we
know this), so you can just pass it onto the hash table, because right now, it
looks odd, you wait dor successful addition to the table (it's not event the
table doing the ref increment), increment ref, return, drop ref - every time I
look at it, it raises the question 'why'...
Erik