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

[Erik Skultety]

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>

Anyhow, based purely on a visual perception (sigh...), it looks like that the
LXC "server" is leaked in lxc_controller.c because virLXCControllerSetupServer
creates the object, calls virNetDaemonAddServer which increments @refcnt, but I
don't see two corresponding virObjectUnrefs (that's the thing, once you don't
run gdb or valgrind, you can't be sure whether this suspicion is right or
not ...).

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.

> +    virObjectUnref(srv);
> +    if (rc < 0) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> @@ -1393,7 +1398,11 @@ int main(int argc, char **argv) {
>          goto cleanup;
>      }
>
> -    if (virNetDaemonAddServer(dmn, srvAdm) < 0) {
> +    /* Add @srvAdm to @dmn servers hash table and Unref to allow removal from
> +     * hash table at @dmn disposal to decide when to free @srvAdm */
> +    rc = virNetDaemonAddServer(dmn, srvAdm);
> +    virObjectUnref(srvAdm);
> +    if (rc < 0) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> @@ -1509,8 +1518,6 @@ int main(int argc, char **argv) {
>      virObjectUnref(qemuProgram);
>      virObjectUnref(adminProgram);
>      virNetDaemonClose(dmn);
> -    virObjectUnref(srv);
> -    virObjectUnref(srvAdm);

^This hunk is of course fine.

Erik