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

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(a)virtuozzo.com&gt; >>>> >>>> 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(a)redhat.com&gt; >>>> --- >>>> 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list >