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

[John Ferlan]

On 10/27/2017 02:29 AM, Nikolay Shirokovskiy wrote:
> 
> 
> 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.
> 

My point wasn't to blame commit 252610f7d entirely, but it helps point
out where the logic was added to use hash tables instead of linked lists.

Still true though that for virNetDaemonAddServer we've ref'd when adding
to the hash table and for virNetDaemonAddServerPostExec we do not.

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

You have to read my last response to your patch, but again...  Consider
that for the virLockDaemonNew path (e.g. rv == 0), there are two refs
created - 1 for the alloc and 1 for the insert to hash table.  When
virNetDaemonGetServer is called a 3rd ref is generated.   So when
"undo-ing" your refs - you need to cover all your bases. For any failure
after virNetDaemonGetServer succeeds, one needs to virObjectUnref that
@srv ref, hence the new label.

If we reach the cleanup: label, then the virObjectUnref there is the
same as the virObjectUnref in log_daemon.c w/r/t virLogDaemonFree and
the virObjectUnref(logd->srv) - that is at this point we're declaring
we're done with @srv.  If there's still a ref because
virNetDaemonDispose hasn't run, then when that runs it'll free things;
otherwise, @srv would be free'd on our Unref.



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

And I don't believe that's necessary.  What purpose does it serve?  When
we leave virLockDaemonNewPostExecRestart or
virNetDaemonAddServerPostExec there should be 2 refs *just like* there
were when we left virLockDaemonNew. That way when we reach the "else (rv
== 1)" code (back in main) and call virNetDaemonGetServer we create that
3rd ref to @srv just like we had in the rv == 0 path.

If virLockDaemonPostExecRestart fails, eg, rv < 0, then all bets are
off, the @srv == NULL, the virObjectUnref does nothing and we die quite
ungracefully.

At this point in time virLockDaemonPostExecRestart only ever returns -1,
0, or 1.

Hopefully this makes sense.

John
>>
>> 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;
>>