[libvirt] [PATCH v3 2/2] virtlogd: add missing netserver refcount increment on reload

[John Ferlan]

On 10/25/2017 05:15 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 25.10.2017 12:06, John Ferlan wrote:
>>
>>
>> On 10/24/2017 06:47 AM, Nikolay Shirokovskiy wrote:
>>> After virNetDaemonAddServerPostExec call in virtlogd we should have
>>> netserver refcount set to 2. One goes to netdaemon servers hashtable
>>> and one goes to virtlogd own reference to netserver. Let's add
>>> missing increment in virNetDaemonAddServerPostExec itself while holding
>>> daemon lock.
>>>
>>> We also have to unref new extra ref after virtlockd call to virNetDaemonAddServerPostExec.
>>> ---
>>>  src/locking/lock_daemon.c | 1 +
>>>  src/rpc/virnetdaemon.c    | 1 +
>>>  2 files changed, 2 insertions(+)
>>>
>>> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
>>> index fe3eaf9..41a06b2 100644
>>> --- a/src/locking/lock_daemon.c
>>> +++ b/src/locking/lock_daemon.c
>>> @@ -275,6 +275,7 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged)
>>>                                                virLockDaemonClientFree,
>>>                                                (void*)(intptr_t)(privileged ? 0x1 : 0x0))))
>>>          goto error;
>>> +    virObjectUnref(srv);
>>
>> I need to think a bit more about this one... different model in lockd
>> vs. logd over @srv usage especially w/r/t this particular path.
>>
>> I see that in this path eventually something calls virNetDaemonGetServer
>> instead of storing the @srv in order to add lockProgram... In any case,> I guess I'd be worried/concerned that something could remove @srv
>> causing the Hash table code to unref/delete the srv... Also, in the
>> cleanup: path of main() wouldn't the Unref(srv) cause problems?
> 
> But this unref only balance newly added ref. If there are other
> problems with reference conting in virtlockd - its a different
> concern.
> 

Ok - so what I've learned is that virLockDaemonPostExecRestart returns
rv == 1 which indicates a successful restore resulting in a call to
virNetDaemonGetServer which will do a virObjectRef anyway leaving us
with (theoretically) 2 refs prior to the code that adds programs to @srv
(and less concern from my part of the subsequent Unref in cleanup: here).

If we saved srv in lockd like logd does, then we wouldn't need to call
virNetDaemonGetServer resulting in the same eventual result except for a
window after the Unref here where the refcnt == 1 until
virNetDaemonGetServer is run. Since I believe nothing could happen in
between that would cause the Unref due to HashFree being called, then I
think we're OK.

The concern being is if virNetDaemonGetServer didn't find the server,
then @srv would be NULL and the subsequent call to
virNetServerAddProgram for lockProgram would fail miserably, but I don't
think that can happen theoretically at least!

John


>>
>> Again- need to think a bit longer.
>>
>> John
>>
>>>  
>>>      return lockd;
>>>  
>>> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
>>> index 495bc4b..f3e3ffe 100644
>>> --- a/src/rpc/virnetdaemon.c
>>> +++ b/src/rpc/virnetdaemon.c
>>> @@ -312,6 +312,7 @@ virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
>>>  
>>>      if (virHashAddEntry(dmn->servers, serverName, srv) < 0)
>>>          goto error;
>>> +    virObjectRef(srv);
>>>  
>>>      virJSONValueFree(object);
>>>      virObjectUnlock(dmn);
>>>