[libvirt] [PATCH v4 1/2] rpc: When adding srv to dmn servers, need to add ref

[Nikolay Shirokovskiy]

On 27.10.2017 08:26, John Ferlan wrote:
> From: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> 
> Commit id '252610f7d' modified net server management to use a
> hash table to store/manage the various servers; however, during
> virNetDaemonAddServerPostExec an @srv object is created, added
> to the dmn->servers hash table, but did not increment the object
> refcnt like was done during virNetDaemonAddServer as if @srv
> were being created for the first time.

I'm not agree that 252610f7d introduced the problem. Before this
commit the situation was the same as now. virNetDaemonAddServer
takes extra reference, virNetDaemonAddServerPostExec does not
take extra reference, virNetDaemonDispose unref every server
in array (just as hash table does). lock daemon does not store
object returned by virNetDaemonAddServerPostExec and log daemon
store the object and unref it in virLogDaemonFree.

> 
> Without the proper ref a subsequent virObjectUnref done during
> virNetDaemonDispose when virHashFree calls the hash table entry
> @dataFree function virObjectFreeHashData could inadvertently free
> the memory before some caller is really done with it or cause the
> caller to attempt to free something that's already been freed (and
> it no longer necessarily owns).
> 
> Additionally, since commit id 'f08b1c58f3' removed the @srv
> from virLockManager in favor of using virNetDaemonGetServer
> which creates a ref on @srv, we need to modify the lock_manager
> code in order to properly handle and dispose the references
> to the @srv object allowing either the cleanup processing or
> the virNetDaemonDispose processing to remove the last ref to
> the object.

But @srv is unrefed on cleanup already. What use of creating
extra label?

Besides this patch is missing unref @srv in virLockDaemonNewPostExecRestart
in the original patch. I think it is necessary.

> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/locking/lock_daemon.c | 19 ++++++++++++++-----
>  src/rpc/virnetdaemon.c    |  2 ++
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index fe3eaf9032..ee2c35fdb8 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -1312,14 +1312,14 @@ int main(int argc, char **argv) {
>          srv = virNetDaemonGetServer(lockDaemon->dmn, "virtlockd");
>          if ((rv = virLockDaemonSetupNetworkingSystemD(srv)) < 0) {
>              ret = VIR_LOCK_DAEMON_ERR_NETWORK;
> -            goto cleanup;
> +            goto cleanup_srvref;
>          }
>  
>          /* Only do this, if systemd did not pass a FD */
>          if (rv == 0 &&
>              virLockDaemonSetupNetworkingNative(srv, sock_file) < 0) {
>              ret = VIR_LOCK_DAEMON_ERR_NETWORK;
> -            goto cleanup;
> +            goto cleanup_srvref;
>          }
>      } else if (rv == 1) {
>          srv = virNetDaemonGetServer(lockDaemon->dmn, "virtlockd");
> @@ -1333,7 +1333,7 @@ int main(int argc, char **argv) {
>  
>      if ((virLockDaemonSetupSignals(lockDaemon->dmn)) < 0) {
>          ret = VIR_LOCK_DAEMON_ERR_SIGNAL;
> -        goto cleanup;
> +        goto cleanup_srvref;
>      }
>  
>      if (!(lockProgram = virNetServerProgramNew(VIR_LOCK_SPACE_PROTOCOL_PROGRAM,
> @@ -1341,14 +1341,17 @@ int main(int argc, char **argv) {
>                                                 virLockSpaceProtocolProcs,
>                                                 virLockSpaceProtocolNProcs))) {
>          ret = VIR_LOCK_DAEMON_ERR_INIT;
> -        goto cleanup;
> +        goto cleanup_srvref;
>      }
>  
>      if (virNetServerAddProgram(srv, lockProgram) < 0) {
>          ret = VIR_LOCK_DAEMON_ERR_INIT;
> -        goto cleanup;
> +        goto cleanup_srvref;
>      }
>  
> +    /* Completed srv usage from virNetDaemonGetServer */
> +    virObjectUnref(srv);
> +
>      /* Disable error func, now logging is setup */
>      virSetErrorFunc(NULL, virLockDaemonErrorHandler);
>  
> @@ -1403,4 +1406,10 @@ int main(int argc, char **argv) {
>   no_memory:
>      VIR_ERROR(_("Can't allocate memory"));
>      exit(EXIT_FAILURE);
> +
> + cleanup_srvref:
> +    /* Unref for virNetDaemonGetServer ref - still have "our" ref from
> +     * allocation and possibly a ref for being in netserver hash table */
> +    virObjectUnref(srv);
> +    goto cleanup;
>  }
> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
> index e3b9390af2..d970c47ad4 100644
> --- a/src/rpc/virnetdaemon.c
> +++ b/src/rpc/virnetdaemon.c
> @@ -313,6 +313,8 @@ virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
>      if (virHashAddEntry(dmn->servers, serverName, srv) < 0)
>          goto error;
>  
> +    virObjectRef(srv);
> +
>      virJSONValueFree(object);
>      virObjectUnlock(dmn);
>      return srv;
>