On 10/27/2017 08:02 AM, Nikolay Shirokovskiy wrote:
>
>
> 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.
>>
I suppose that's another way to handle this since virLockDaemonNew
allocates and places into hash table and then "theoretically" becomes
hands off allowing the main() to call virNetDaemonGetServer in order to
get a new ref to the object. The virLogDaemon handling should do the
same thing IMO - that is don't save @srv in logd - but that's a
different issue.
Also of note is that @srv is leaked when going to error: in
virLockDaemonNew if virNetDaemonNew fails <sigh>.
I do see now why you also had the Unref patch in patch2 of v2.
I'll hopefully talk with Erik in the morning and we can come to a
conclusion on this.
John
>>>
>>>
>>>
>>>> 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
