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

[Erik Skultety]

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 at 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