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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 27.10.2017 14:29, Nikolay Shirokovskiy wrote:
> 
> 
> On 27.10.2017 13:44, John Ferlan wrote:
>>
>>
>> On 10/27/2017 02:29 AM, Nikolay Shirokovskiy wrote:
>>>
>>>
>>> On 27.10.2017 08:26, John Ferlan wrote:
>>>> From: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
>>>>
>>>> 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.
> 
> Ohh, I see know. You adressing another problem in virtlockd code. virLockDaemonNew
> forgets to unref @srv. I think this should be better addressed in virLockDaemonNew.
> 
>>
>>
>>
>>> 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.
> 
> Or we can fix virLockDaemonNew to have only 1 refs. 1 ref is enough AFAIU.
> 
>>
>> If virLockDaemonPostExecRestart fails, eg, rv < 0, then all bets are
>> off, the @srv == NULL, the virObjectUnref does nothing and we die quite
>> ungracefully.
> 
> I think we better make virLockDaemonPostExecRestart to return with no references
> to net servers in case of fail then we have no problems.
> 
>>
>> At this point in time virLockDaemonPostExecRestart only ever returns -1,
>> 0, or 1.
>>
>> Hopefully this makes sense.
>>
> 
> In short I would leave unref in virLockDaemonPostExecRestart of the original
> patch and add unref to virLockDaemonNew in a distinct patch.

Particularly the former missing unref is introduced in fa1420736 and the latter is in
f08b1c58f3.

> 
>>>>
>>>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>>>> ---
>>>>  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;
>>>>
> 
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux