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