Whether the @srv/@srvAdm is added to the dmn->servers list or not,
the reference kept for the allocation can be dropped leaving just the
reference for being on the dmn->servers list be the sole deciding
factor when to really free the associated memory. The @dmn dispose
function (virNetDaemonDispose) will handle the Unref of the objects
during the virHashFree.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 daemon/libvirtd.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 73f24915df..5c47e49d48 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1065,6 +1065,7 @@ int main(int argc, char **argv) {
     char *remote_config_file = NULL;
     int statuswrite = -1;
     int ret = 1;
+    int rc;
     int pid_file_fd = -1;
     char *pid_file = NULL;
     char *sock_file = NULL;
@@ -1319,7 +1320,11 @@ int main(int argc, char **argv) {
         goto cleanup;
     }
 
-    if (virNetDaemonAddServer(dmn, srv) < 0) {
+    /* Add @srv to @dmn servers hash table and Unref to allow removal from
+     * hash table at @dmn disposal to decide when to free @srv */
+    rc = virNetDaemonAddServer(dmn, srv);
+    virObjectUnref(srv);
+    if (rc < 0) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
@@ -1393,7 +1398,11 @@ int main(int argc, char **argv) {
         goto cleanup;
     }
 
-    if (virNetDaemonAddServer(dmn, srvAdm) < 0) {
+    /* Add @srvAdm to @dmn servers hash table and Unref to allow removal from
+     * hash table at @dmn disposal to decide when to free @srvAdm */
+    rc = virNetDaemonAddServer(dmn, srvAdm);
+    virObjectUnref(srvAdm);
+    if (rc < 0) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
@@ -1509,8 +1518,6 @@ int main(int argc, char **argv) {
     virObjectUnref(qemuProgram);
     virObjectUnref(adminProgram);
     virNetDaemonClose(dmn);
-    virObjectUnref(srv);
-    virObjectUnref(srvAdm);
     virNetlinkShutdown();
     if (statuswrite != -1) {
         if (ret != 0) {
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Nov 07, 2017 at 09:39:54PM -0500, John Ferlan wrote:
> Whether the @srv/@srvAdm is added to the dmn->servers list or not,
> the reference kept for the allocation can be dropped leaving just the
> reference for being on the dmn->servers list be the sole deciding
> factor when to really free the associated memory. The @dmn dispose
> function (virNetDaemonDispose) will handle the Unref of the objects
> during the virHashFree.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  daemon/libvirtd.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 73f24915df..5c47e49d48 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -1065,6 +1065,7 @@ int main(int argc, char **argv) {
>      char *remote_config_file = NULL;
>      int statuswrite = -1;
>      int ret = 1;
> +    int rc;
>      int pid_file_fd = -1;
>      char *pid_file = NULL;
>      char *sock_file = NULL;
> @@ -1319,7 +1320,11 @@ int main(int argc, char **argv) {
>          goto cleanup;
>      }
>
> -    if (virNetDaemonAddServer(dmn, srv) < 0) {
> +    /* Add @srv to @dmn servers hash table and Unref to allow removal from
> +     * hash table at @dmn disposal to decide when to free @srv */
> +    rc = virNetDaemonAddServer(dmn, srv);

<rant>
Sorry for the delay, I've been playing with LXC for 2 days, trying to either
debug or just use valgrind on lxc_controller to get the info I need
(explanation below), but after those 2 days, I just give up, not worth any more
time, if someone knows how I can actually hit the cleanup section in
lxc_controller within gdb telling me that it suddenly temporarily disabled
breakpoints at the moment it's supposed to stop at one that it had let me to set
earlier, I'm all ears. Btw to my biggest surprise valgrind still doesn't handle
setns syscall which has probably been the reason why we forbade running LXC
under valgrind in 2009, unbelievable. </rant>

Anyhow, based purely on a visual perception (sigh...), it looks like that the
LXC "server" is leaked in lxc_controller.c because virLXCControllerSetupServer
creates the object, calls virNetDaemonAddServer which increments @refcnt, but I
don't see two corresponding virObjectUnrefs (that's the thing, once you don't
run gdb or valgrind, you can't be sure whether this suspicion is right or
not ...).

Now the actual review.
virNetDaemonAddServer is only used when spawning a new daemon or setting up LXC
controller. The function essentially does:

lock(@dmn)
    hash_table_add(@srv)
    ref(@srv)
unlock(@dmn)

and then you unref @dmn right upon the completion of adding the server to the
hash table, what's the purpose of the extra ref when you discard it
immediately? Unless I'm failing to see something, I'd prefer to just drop the
extra ref in virNetDaemonAddServer and leave the proposed hunks unchanged - you
have 1 ref which you got by creating the object, now it should be just simply
inserted into the hash table, the additional ref after insertion doesn't make
sense, if someone managed to unref it before inserting it into the hash table,
hash insert would most likely fail, if not, hashFree surely would, not
mentioning that at that point there's no entity that is touching servers.

> +    virObjectUnref(srv);
> +    if (rc < 0) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> @@ -1393,7 +1398,11 @@ int main(int argc, char **argv) {
>          goto cleanup;
>      }
>
> -    if (virNetDaemonAddServer(dmn, srvAdm) < 0) {
> +    /* Add @srvAdm to @dmn servers hash table and Unref to allow removal from
> +     * hash table at @dmn disposal to decide when to free @srvAdm */
> +    rc = virNetDaemonAddServer(dmn, srvAdm);
> +    virObjectUnref(srvAdm);
> +    if (rc < 0) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> @@ -1509,8 +1518,6 @@ int main(int argc, char **argv) {
>      virObjectUnref(qemuProgram);
>      virObjectUnref(adminProgram);
>      virNetDaemonClose(dmn);
> -    virObjectUnref(srv);
> -    virObjectUnref(srvAdm);

^This hunk is of course fine.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 11/10/2017 10:08 AM, Erik Skultety wrote:
> On Tue, Nov 07, 2017 at 09:39:54PM -0500, John Ferlan wrote:
>> Whether the @srv/@srvAdm is added to the dmn->servers list or not,
>> the reference kept for the allocation can be dropped leaving just the
>> reference for being on the dmn->servers list be the sole deciding
>> factor when to really free the associated memory. The @dmn dispose
>> function (virNetDaemonDispose) will handle the Unref of the objects
>> during the virHashFree.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  daemon/libvirtd.c | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
>> index 73f24915df..5c47e49d48 100644
>> --- a/daemon/libvirtd.c
>> +++ b/daemon/libvirtd.c
>> @@ -1065,6 +1065,7 @@ int main(int argc, char **argv) {
>>      char *remote_config_file = NULL;
>>      int statuswrite = -1;
>>      int ret = 1;
>> +    int rc;
>>      int pid_file_fd = -1;
>>      char *pid_file = NULL;
>>      char *sock_file = NULL;
>> @@ -1319,7 +1320,11 @@ int main(int argc, char **argv) {
>>          goto cleanup;
>>      }
>>
>> -    if (virNetDaemonAddServer(dmn, srv) < 0) {
>> +    /* Add @srv to @dmn servers hash table and Unref to allow removal from
>> +     * hash table at @dmn disposal to decide when to free @srv */
>> +    rc = virNetDaemonAddServer(dmn, srv);
> 
> <rant>
> Sorry for the delay, I've been playing with LXC for 2 days, trying to either
> debug or just use valgrind on lxc_controller to get the info I need
> (explanation below), but after those 2 days, I just give up, not worth any more
> time, if someone knows how I can actually hit the cleanup section in
> lxc_controller within gdb telling me that it suddenly temporarily disabled
> breakpoints at the moment it's supposed to stop at one that it had let me to set
> earlier, I'm all ears. Btw to my biggest surprise valgrind still doesn't handle
> setns syscall which has probably been the reason why we forbade running LXC
> under valgrind in 2009, unbelievable. </rant>

I feel your pain ;-)  Chasing these memory things is challenging
especially when variable names change or things just aren't consistent.
They're a real time sync too - that whole NetDaemon, NetServer,
NetServerClient, etc. code is a beast to understand.  I only have
figured out a small piece of it...

> 
> Anyhow, based purely on a visual perception (sigh...), it looks like that the
> LXC "server" is leaked in lxc_controller.c because virLXCControllerSetupServer
> creates the object, calls virNetDaemonAddServer which increments @refcnt, but I
> don't see two corresponding virObjectUnrefs (that's the thing, once you don't
> run gdb or valgrind, you can't be sure whether this suspicion is right or
> not ...).

So let's see - I see the 2 REF's on @srv as you do in main().  The
ctrl->daemon->servers will be UNREF'd in virNetDaemonDispose when the
last Unref of ctrl->daemon is done in virLXCControllerFree via the call
to virHashFree... BTW: Chasing that is painful, but once the server is
removed from the hash table it does end up calling an Unref when
virObjectFreeHashData is called (e.g. the 2nd arg to virHashCreate).


This may be one of those cases where you need to add "temporary" debug
code to virObjectRef and virObjectUnref along with the address of the
object in order to see "when" it's Ref'd and Unref'd and what the count
is (I've done this using VIR_WARN and just perusing generated output).
It is painful... Don't forget to add debug code to this function that
would print the addr of the daemon and server object's so that you can
output everything to a file and then comb through the output looking for
when things are ref/unref'd and most importantly some free of an object
that didn't have an object it contains being free'd.  Once you see the
daemon object free'd, but the @srv object still hanging on - you know
you're pretty safe in assuming there's a missing Unref...


Anyway, I agree with you upon visual inspection - there's a leak since
@srv is never Unref'd. Once it's added to the Server hash table, then
there should be a virObjectUnref(srv).

As for other consumers.... The reason that log_daemon doesn't do the
virObjectUnref is that it saves @srv and eventually will Unref it later.
Unlike the lock_daemon code which uses virNetDaemonGetServer to find the
@srv; whereas, log_daemon goes direct to logDaemon->srv.


> 
> Now the actual review.
> virNetDaemonAddServer is only used when spawning a new daemon or setting up LXC
> controller. The function essentially does:
> 
> lock(@dmn)
>     hash_table_add(@srv)
>     ref(@srv)
> unlock(@dmn)
> 
> and then you unref @dmn right upon the completion of adding the server to the
> hash table, what's the purpose of the extra ref when you discard it
> immediately? 

The REF is for the Hash Table. We've added something to the hash table -
we should increment the refcnt - nothing more nothing less. The UNREF
for that occurs as part of virHashFree because virHashCreate uses
virObjectFreeHashData.

The UNREF is for the allocation (or virNetServerNew) and an optimization
because we know it's in the Hash Table and wouldn't be removed from the
Hash Table until the virNetDaemonDispose calls virHashFree once the last
@dmn is UNREF'd.

Dropping this patch and the next one is fine by me, but the Unref's
below would be reinstated.

> Unless I'm failing to see something, I'd prefer to just drop the
> extra ref in virNetDaemonAddServer and leave the proposed hunks unchanged - you
> have 1 ref which you got by creating the object, now it should be just simply
> inserted into the hash table, the additional ref after insertion doesn't make
> sense, if someone managed to unref it before inserting it into the hash table,
> hash insert would most likely fail, if not, hashFree surely would, not
> mentioning that at that point there's no entity that is touching servers.

That messes up log_daemon as it's stores @srv in logDamon->srv and then
will virObjectUnref it virLogDaemonFree. If log_daemon didn't save a ref
of the object in logDaemon->srv, then it too could do a virObjectUnref
right away too.

> 
>> +    virObjectUnref(srv);
>> +    if (rc < 0) {
>>          ret = VIR_DAEMON_ERR_INIT;
>>          goto cleanup;
>>      }
>> @@ -1393,7 +1398,11 @@ int main(int argc, char **argv) {
>>          goto cleanup;
>>      }
>>
>> -    if (virNetDaemonAddServer(dmn, srvAdm) < 0) {
>> +    /* Add @srvAdm to @dmn servers hash table and Unref to allow removal from
>> +     * hash table at @dmn disposal to decide when to free @srvAdm */
>> +    rc = virNetDaemonAddServer(dmn, srvAdm);
>> +    virObjectUnref(srvAdm);
>> +    if (rc < 0) {
>>          ret = VIR_DAEMON_ERR_INIT;
>>          goto cleanup;
>>      }
>> @@ -1509,8 +1518,6 @@ int main(int argc, char **argv) {
>>      virObjectUnref(qemuProgram);
>>      virObjectUnref(adminProgram);
>>      virNetDaemonClose(dmn);
>> -    virObjectUnref(srv);
>> -    virObjectUnref(srvAdm);
> 
> ^This hunk is of course fine.
> 
> Erik
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Nov 10, 2017 at 05:41:51PM -0500, John Ferlan wrote:
>
>
> On 11/10/2017 10:08 AM, Erik Skultety wrote:
> > On Tue, Nov 07, 2017 at 09:39:54PM -0500, John Ferlan wrote:
> >> Whether the @srv/@srvAdm is added to the dmn->servers list or not,
> >> the reference kept for the allocation can be dropped leaving just the
> >> reference for being on the dmn->servers list be the sole deciding
> >> factor when to really free the associated memory. The @dmn dispose
> >> function (virNetDaemonDispose) will handle the Unref of the objects
> >> during the virHashFree.
> >>
> >> Signed-off-by: John Ferlan <jferlan@redhat.com>
> >> ---
> >>  daemon/libvirtd.c | 15 +++++++++++----
> >>  1 file changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> >> index 73f24915df..5c47e49d48 100644
> >> --- a/daemon/libvirtd.c
> >> +++ b/daemon/libvirtd.c
> >> @@ -1065,6 +1065,7 @@ int main(int argc, char **argv) {
> >>      char *remote_config_file = NULL;
> >>      int statuswrite = -1;
> >>      int ret = 1;
> >> +    int rc;
> >>      int pid_file_fd = -1;
> >>      char *pid_file = NULL;
> >>      char *sock_file = NULL;
> >> @@ -1319,7 +1320,11 @@ int main(int argc, char **argv) {
> >>          goto cleanup;
> >>      }
> >>
> >> -    if (virNetDaemonAddServer(dmn, srv) < 0) {
> >> +    /* Add @srv to @dmn servers hash table and Unref to allow removal from
> >> +     * hash table at @dmn disposal to decide when to free @srv */
> >> +    rc = virNetDaemonAddServer(dmn, srv);
> >
> > <rant>
> > Sorry for the delay, I've been playing with LXC for 2 days, trying to either
> > debug or just use valgrind on lxc_controller to get the info I need
> > (explanation below), but after those 2 days, I just give up, not worth any more
> > time, if someone knows how I can actually hit the cleanup section in
> > lxc_controller within gdb telling me that it suddenly temporarily disabled
> > breakpoints at the moment it's supposed to stop at one that it had let me to set
> > earlier, I'm all ears. Btw to my biggest surprise valgrind still doesn't handle
> > setns syscall which has probably been the reason why we forbade running LXC
> > under valgrind in 2009, unbelievable. </rant>
>
> I feel your pain ;-)  Chasing these memory things is challenging
> especially when variable names change or things just aren't consistent.

Well, it wouldn't have to be if one didn't have to fight the mentioned tools so
much - if you've been using fedora since v25 (I don't know whether debian/ubuntu
suffer the same way), you might have come across this weird thing that even if
you compile libvirt with debugging symbols, you're not able to actually step
into non-static functions with "step" (you'd have to use stepi multiple times)
because the linker doesn't generate the PLT stubs for some reason, which is
quite frustrating for someone who debugs a lot and more breakpoints just don't
help here really since some of them might get hit constantly from places I don't
want and disabling them simply doesn't feel efficient like using 'step'.

...
> Anyway, I agree with you upon visual inspection - there's a leak since
> @srv is never Unref'd. Once it's added to the Server hash table, then
> there should be a virObjectUnref(srv).

\o/ This is just one in a ... lot ..., since the Unrefs are almost impossible
to track visually... :)

>
> As for other consumers.... The reason that log_daemon doesn't do the
> virObjectUnref is that it saves @srv and eventually will Unref it later.
> Unlike the lock_daemon code which uses virNetDaemonGetServer to find the
> @srv; whereas, log_daemon goes direct to logDaemon->srv.

Yeah, that just doesn't feel right, virtlogd is just a copy of virtlockd, I
haven't found a compelling reason why they're different in this aspect, so I
sent simple patches to address that.

>
>
> >
> > Now the actual review.
> > virNetDaemonAddServer is only used when spawning a new daemon or setting up LXC
> > controller. The function essentially does:
> >
> > lock(@dmn)
> >     hash_table_add(@srv)
> >     ref(@srv)
> > unlock(@dmn)
> >
> > and then you unref @dmn right upon the completion of adding the server to the
> > hash table, what's the purpose of the extra ref when you discard it
> > immediately?
>
> The REF is for the Hash Table. We've added something to the hash table -
> we should increment the refcnt - nothing more nothing less. The UNREF
> for that occurs as part of virHashFree because virHashCreate uses
> virObjectFreeHashData.

Sure, I understand that the ref is for the hash table, but there's no other
caller that would interfere with the object (the idea behind ref counting), you
already have a ref for the daemon, which is not going to need it anymore (we
know this), so you can just pass it onto the hash table, because right now, it
looks odd, you wait dor successful addition to the table (it's not event the
table doing the ref increment), increment ref, return, drop ref - every time I
look at it, it raises the question 'why'...

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

> Now the actual review.
> virNetDaemonAddServer is only used when spawning a new daemon or setting up LXC
> controller. The function essentially does:
> 
> lock(@dmn)
>     hash_table_add(@srv)
>     ref(@srv)
> unlock(@dmn)
> 
> and then you unref @dmn right upon the completion of adding the server to the
> hash table, what's the purpose of the extra ref when you discard it
> immediately? Unless I'm failing to see something, I'd prefer to just drop the
> extra ref in virNetDaemonAddServer and leave the proposed hunks unchanged - you
> have 1 ref which you got by creating the object, now it should be just simply
> inserted into the hash table, the additional ref after insertion doesn't make
> sense, if someone managed to unref it before inserting it into the hash table,
> hash insert would most likely fail, if not, hashFree surely would, not
> mentioning that at that point there's no entity that is touching servers.
> 

Since I was "digging" on a different issue - check out how this is done
elsewhere... Start with virNetServerDispatchNewClient.  It'll call the
ClientNew function which generates a REF.  Then it will call AddClient
which generates a REF. Then because it's on that list and this code is
theoretically done with it - it will UNREF the client before returning.

John

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Sun, Nov 12, 2017 at 09:46:48AM -0500, John Ferlan wrote:
>
> [...]
>
> > Now the actual review.
> > virNetDaemonAddServer is only used when spawning a new daemon or setting up LXC
> > controller. The function essentially does:
> >
> > lock(@dmn)
> >     hash_table_add(@srv)
> >     ref(@srv)
> > unlock(@dmn)
> >
> > and then you unref @dmn right upon the completion of adding the server to the
> > hash table, what's the purpose of the extra ref when you discard it
> > immediately? Unless I'm failing to see something, I'd prefer to just drop the
> > extra ref in virNetDaemonAddServer and leave the proposed hunks unchanged - you
> > have 1 ref which you got by creating the object, now it should be just simply
> > inserted into the hash table, the additional ref after insertion doesn't make
> > sense, if someone managed to unref it before inserting it into the hash table,
> > hash insert would most likely fail, if not, hashFree surely would, not
> > mentioning that at that point there's no entity that is touching servers.
> >
>
> Since I was "digging" on a different issue - check out how this is done
> elsewhere... Start with virNetServerDispatchNewClient.  It'll call the
> ClientNew function which generates a REF.  Then it will call AddClient
> which generates a REF. Then because it's on that list and this code is
> theoretically done with it - it will UNREF the client before returning.

Sure, but we shouldn't treat everything in a uniform manner - the fact that we
can do that and it works still doesn't necessarily mean it's right. BTW I only
looked at NetClient briefly, but that too looks to me like the reffing is
unnecessary.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 11/14/2017 09:17 AM, Erik Skultety wrote:
> On Sun, Nov 12, 2017 at 09:46:48AM -0500, John Ferlan wrote:
>>
>> [...]
>>
>>> Now the actual review.
>>> virNetDaemonAddServer is only used when spawning a new daemon or setting up LXC
>>> controller. The function essentially does:
>>>
>>> lock(@dmn)
>>>     hash_table_add(@srv)
>>>     ref(@srv)
>>> unlock(@dmn)
>>>
>>> and then you unref @dmn right upon the completion of adding the server to the
>>> hash table, what's the purpose of the extra ref when you discard it
>>> immediately? Unless I'm failing to see something, I'd prefer to just drop the
>>> extra ref in virNetDaemonAddServer and leave the proposed hunks unchanged - you
>>> have 1 ref which you got by creating the object, now it should be just simply
>>> inserted into the hash table, the additional ref after insertion doesn't make
>>> sense, if someone managed to unref it before inserting it into the hash table,
>>> hash insert would most likely fail, if not, hashFree surely would, not
>>> mentioning that at that point there's no entity that is touching servers.
>>>
>>
>> Since I was "digging" on a different issue - check out how this is done
>> elsewhere... Start with virNetServerDispatchNewClient.  It'll call the
>> ClientNew function which generates a REF.  Then it will call AddClient
>> which generates a REF. Then because it's on that list and this code is
>> theoretically done with it - it will UNREF the client before returning.
> 
> Sure, but we shouldn't treat everything in a uniform manner - the fact that we
> can do that and it works still doesn't necessarily mean it's right. BTW I only
> looked at NetClient briefly, but that too looks to me like the reffing is
> unnecessary.
> 
> Erik
> 

I do understand your point, but undoing the Ref when placing into the
HashTable consistently across all consumers is perhaps a large task.

At this point, I'm thinking I just drop patches 3 & 4 - it just means
the objects will have 2 refs and allows me/us to just make forward
progress putting the discussion about the need to Ref when adding to the
hash table for a different day or set of patches.  In the end the more
important patch is 5 - at least with respect to does it handle the issue
that Nikolay was originally trying to handle.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Nov 15, 2017 at 05:59:39PM -0500, John Ferlan wrote:
>
>
> On 11/14/2017 09:17 AM, Erik Skultety wrote:
> > On Sun, Nov 12, 2017 at 09:46:48AM -0500, John Ferlan wrote:
> >>
> >> [...]
> >>
> >>> Now the actual review.
> >>> virNetDaemonAddServer is only used when spawning a new daemon or setting up LXC
> >>> controller. The function essentially does:
> >>>
> >>> lock(@dmn)
> >>>     hash_table_add(@srv)
> >>>     ref(@srv)
> >>> unlock(@dmn)
> >>>
> >>> and then you unref @dmn right upon the completion of adding the server to the
> >>> hash table, what's the purpose of the extra ref when you discard it
> >>> immediately? Unless I'm failing to see something, I'd prefer to just drop the
> >>> extra ref in virNetDaemonAddServer and leave the proposed hunks unchanged - you
> >>> have 1 ref which you got by creating the object, now it should be just simply
> >>> inserted into the hash table, the additional ref after insertion doesn't make
> >>> sense, if someone managed to unref it before inserting it into the hash table,
> >>> hash insert would most likely fail, if not, hashFree surely would, not
> >>> mentioning that at that point there's no entity that is touching servers.
> >>>
> >>
> >> Since I was "digging" on a different issue - check out how this is done
> >> elsewhere... Start with virNetServerDispatchNewClient.  It'll call the
> >> ClientNew function which generates a REF.  Then it will call AddClient
> >> which generates a REF. Then because it's on that list and this code is
> >> theoretically done with it - it will UNREF the client before returning.
> >
> > Sure, but we shouldn't treat everything in a uniform manner - the fact that we
> > can do that and it works still doesn't necessarily mean it's right. BTW I only
> > looked at NetClient briefly, but that too looks to me like the reffing is
> > unnecessary.
> >
> > Erik
> >
>
> I do understand your point, but undoing the Ref when placing into the
> HashTable consistently across all consumers is perhaps a large task.
>
> At this point, I'm thinking I just drop patches 3 & 4 - it just means
> the objects will have 2 refs and allows me/us to just make forward
> progress putting the discussion about the need to Ref when adding to the

OK, we can do that...

> hash table for a different day or set of patches.  In the end the more
> important patch is 5 - at least with respect to does it handle the issue
> that Nikolay was originally trying to handle.

Damn, I forgot to respond to patch 5, sorry about that, I just did.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list