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.
>>>
>>> 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
