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); >>> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
