3:31 p.m.
Domain and Net objects were not being cleaned up properly
when reporting errors from the remote driver. Attached patch
fixes this.
Thanks,
Cole
diff --git a/src/remote_internal.c b/src/remote_internal.c
index 51e8eb7..80f6ce6 100644
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -4606,6 +4606,10 @@ server_error (virConnectPtr conn, remote_error *err)
err->str3 ? *err->str3 : NULL,
err->int1, err->int2,
"%s", err->message ? *err->message : NULL);
+ if (dom)
+ virDomainFree(dom);
+ if (net)
+ virNetworkFree(dom);
}
/* get_nonnull_domain and get_nonnull_network turn an on-wire
3:58 p.m.
Cole Robinson wrote:
Domain and Net objects were not being cleaned up properly
when reporting errors from the remote driver. Attached patch
fixes this.
Stupid typo. Correct patch attached.
- Cole
diff --git a/src/remote_internal.c b/src/remote_internal.c
index 51e8eb7..80f6ce6 100644
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -4606,6 +4606,10 @@ server_error (virConnectPtr conn, remote_error *err)
err->str3 ? *err->str3 : NULL,
err->int1, err->int2,
"%s", err->message ? *err->message : NULL);
+ if (dom)
+ virDomainFree(dom);
+ if (net)
+ virNetworkFree(net);
}
/* get_nonnull_domain and get_nonnull_network turn an on-wire
4:44 a.m.
On Mon, May 19, 2008 at 04:58:38PM -0400, Cole Robinson wrote:
diff --git a/src/remote_internal.c b/src/remote_internal.c
index 51e8eb7..80f6ce6 100644
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -4606,6 +4606,10 @@ server_error (virConnectPtr conn, remote_error *err)
err->str3 ? *err->str3 : NULL,
err->int1, err->int2,
"%s", err->message ? *err->message : NULL);
+ if (dom)
+ virDomainFree(dom);
+ if (net)
+ virNetworkFree(net);
}
Extra long hmmmmmmmmmmm ...
This is right, the domain and network are leaked.
However the virterror structure containing these pointers will be used
later. (__virRaiseError saves it and callers access it later).
However^2 these entries in the virterror structure are deprecated. I
added a patch last month which adds a big warning about using these
entries. At most callers should look at the pointers themselves and
shouldn't dereference them (which would be the only safe thing to do
given that the pointers have just been freed).
But maybe there is old code around using the dom/net fields in
virterror ... It's a bad idea even to look at those fields really.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
7:02 a.m.
On Tue, May 20, 2008 at 10:44:52AM +0100, Richard W.M. Jones wrote:
On Mon, May 19, 2008 at 04:58:38PM -0400, Cole Robinson wrote:
> diff --git a/src/remote_internal.c b/src/remote_internal.c
> index 51e8eb7..80f6ce6 100644
> --- a/src/remote_internal.c
> +++ b/src/remote_internal.c
> @@ -4606,6 +4606,10 @@ server_error (virConnectPtr conn, remote_error *err)
> err->str3 ? *err->str3 : NULL,
> err->int1, err->int2,
> "%s", err->message ? *err->message : NULL);
> + if (dom)
> + virDomainFree(dom);
> + if (net)
> + virNetworkFree(net);
> }
Extra long hmmmmmmmmmmm ...
This is right, the domain and network are leaked.
However the virterror structure containing these pointers will be used
later. (__virRaiseError saves it and callers access it later).
However^2 these entries in the virterror structure are deprecated. I
added a patch last month which adds a big warning about using these
entries. At most callers should look at the pointers themselves and
shouldn't dereference them (which would be the only safe thing to do
given that the pointers have just been freed).
Yes, that should be safe - the only domain / network object set in an error
condition is one that is passed in via the original caller. So there should
still be a reference count held on these objects further up the call stack
and thus this code won't actually free them immediately, merely decrement
the ref count. Can probably confirm this by turning on debug mode and checking
the messages.
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
8:32 a.m.
Daniel P. Berrange wrote:
On Tue, May 20, 2008 at 10:44:52AM +0100, Richard W.M. Jones wrote:
> On Mon, May 19, 2008 at 04:58:38PM -0400, Cole Robinson wrote:
>> diff --git a/src/remote_internal.c b/src/remote_internal.c
>> index 51e8eb7..80f6ce6 100644
>> --- a/src/remote_internal.c
>> +++ b/src/remote_internal.c
>> @@ -4606,6 +4606,10 @@ server_error (virConnectPtr conn, remote_error *err)
>> err->str3 ? *err->str3 : NULL,
>> err->int1, err->int2,
>> "%s", err->message ? *err->message :
NULL);
>> + if (dom)
>> + virDomainFree(dom);
>> + if (net)
>> + virNetworkFree(net);
>> }
> Extra long hmmmmmmmmmmm ...
>
> This is right, the domain and network are leaked.
>
> However the virterror structure containing these pointers will be used
> later. (__virRaiseError saves it and callers access it later).
>
> However^2 these entries in the virterror structure are deprecated. I
> added a patch last month which adds a big warning about using these
> entries. At most callers should look at the pointers themselves and
> shouldn't dereference them (which would be the only safe thing to do
> given that the pointers have just been freed).
Yes, that should be safe - the only domain / network object set in an error
condition is one that is passed in via the original caller. So there should
still be a reference count held on these objects further up the call stack
and thus this code won't actually free them immediately, merely decrement
the ref count. Can probably confirm this by turning on debug mode and checking
the messages.
Dan.
I just tested without this patch, but with the other destroy leak fixes I
sent, and everything looks to be okay. So this patch can be ignored.
Thanks,
Cole
8:39 a.m.
On Tue, May 20, 2008 at 09:32:28AM -0400, Cole Robinson wrote:
Daniel P. Berrange wrote:
> On Tue, May 20, 2008 at 10:44:52AM +0100, Richard W.M. Jones wrote:
>> On Mon, May 19, 2008 at 04:58:38PM -0400, Cole Robinson wrote:
>>> diff --git a/src/remote_internal.c b/src/remote_internal.c
>>> index 51e8eb7..80f6ce6 100644
>>> --- a/src/remote_internal.c
>>> +++ b/src/remote_internal.c
>>> @@ -4606,6 +4606,10 @@ server_error (virConnectPtr conn, remote_error *err)
>>> err->str3 ? *err->str3 : NULL,
>>> err->int1, err->int2,
>>> "%s", err->message ? *err->message :
NULL);
>>> + if (dom)
>>> + virDomainFree(dom);
>>> + if (net)
>>> + virNetworkFree(net);
>>> }
>> Extra long hmmmmmmmmmmm ...
>>
>> This is right, the domain and network are leaked.
>>
>> However the virterror structure containing these pointers will be used
>> later. (__virRaiseError saves it and callers access it later).
>>
>> However^2 these entries in the virterror structure are deprecated. I
>> added a patch last month which adds a big warning about using these
>> entries. At most callers should look at the pointers themselves and
>> shouldn't dereference them (which would be the only safe thing to do
>> given that the pointers have just been freed).
>
> Yes, that should be safe - the only domain / network object set in an error
> condition is one that is passed in via the original caller. So there should
> still be a reference count held on these objects further up the call stack
> and thus this code won't actually free them immediately, merely decrement
> the ref count. Can probably confirm this by turning on debug mode and checking
> the messages.
I just tested without this patch, but with the other destroy leak fixes I
sent, and everything looks to be okay. So this patch can be ignored.
I'm not entirely convinced yet - the code certainly suggests to me that
we need to free these. Only a couple of lines further up we obtain a referenced
object
/* Get the domain and network, if set. */
dom = err->dom ? get_nonnull_domain (conn, *err->dom) : NULL;
net = err->net ? get_nonnull_network (conn, *err->net) : NULL;
And __virRaiseError() does *not* own the reference after being called, thus
we need to explicitly release the reference ourselves.
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
8:39 a.m.
On Tue, May 20, 2008 at 02:39:52PM +0100, Daniel P. Berrange wrote:
I'm not entirely convinced yet - the code certainly suggests to
me that
we need to free these. Only a couple of lines further up we obtain a referenced
object
/* Get the domain and network, if set. */
dom = err->dom ? get_nonnull_domain (conn, *err->dom) : NULL;
net = err->net ? get_nonnull_network (conn, *err->net) : NULL;
And __virRaiseError() does *not* own the reference after being called, thus
we need to explicitly release the reference ourselves.
Agreed with Dan ... I'm not sure how the leaks can go away with the
other patch. OTOH the ref-counting stuff is complicated.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
8:57 a.m.
Richard W.M. Jones wrote:
On Tue, May 20, 2008 at 02:39:52PM +0100, Daniel P. Berrange wrote:
> I'm not entirely convinced yet - the code certainly suggests to me that
> we need to free these. Only a couple of lines further up we obtain a referenced
> object
>
> /* Get the domain and network, if set. */
> dom = err->dom ? get_nonnull_domain (conn, *err->dom) : NULL;
> net = err->net ? get_nonnull_network (conn, *err->net) : NULL;
>
> And __virRaiseError() does *not* own the reference after being called, thus
> we need to explicitly release the reference ourselves.
Agreed with Dan ... I'm not sure how the leaks can go away with the
other patch. OTOH the ref-counting stuff is complicated.
Rich.
I haven't really traced the code path in detail to figure out what
was taking place but I'll explain what I was seeing:
Connect to qemu driver with virsh. I have one defined vm currently
shutoff. Trying to 'shutdown' the vm seems to cause no reference
count leaks, and I can do it until I'm blue in the face. But once
I try to 'destroy' the domain, a reference leak pops up. Now every
subsequent 'destroy' or 'shutdown' command creates another leak (I
didn't try any other commands). I thought I traced it to the error
and remote.c leaks, and when fixed sequentially all problems went
away. But now I'm not so sure what state all the code was in.
I'll try this again from the original unpatched state and try to
figure out exactly what was happening.
9:01 a.m.
On Tue, May 20, 2008 at 09:57:02AM -0400, Cole Robinson wrote:
I'll try this again from the original unpatched state and try to
figure out exactly what was happening.
Do you trigger an error at any point in the testing? You could try
doing that (eg. try migrating a QEMU domain).
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
9:11 a.m.
Richard W.M. Jones wrote:
On Tue, May 20, 2008 at 09:57:02AM -0400, Cole Robinson wrote:
> I'll try this again from the original unpatched state and try to
> figure out exactly what was happening.
Do you trigger an error at any point in the testing? You could try
doing that (eg. try migrating a QEMU domain).
Rich.
Yes the whole time I am triggering errors, as I am trying to shutdown
or destroy an already shutdown domain. That's the interesting part:
once destroy errors once, even subsequent shutdown errors start leaking.
- Cole
9:38 a.m.
On Tue, May 20, 2008 at 10:11:57AM -0400, Cole Robinson wrote:
Richard W.M. Jones wrote:
> On Tue, May 20, 2008 at 09:57:02AM -0400, Cole Robinson wrote:
>> I'll try this again from the original unpatched state and try to
>> figure out exactly what was happening.
>
> Do you trigger an error at any point in the testing? You could try
> doing that (eg. try migrating a QEMU domain).
Yes the whole time I am triggering errors, as I am trying to shutdown
or destroy an already shutdown domain. That's the interesting part:
once destroy errors once, even subsequent shutdown errors start leaking.
Ok, so the problem is a very complex one and here's what's going on....
- Storing the domain/network objects in the virError struct does not
increment the reference count.
- When a domain ref count drops to zero, the virReleaseDomain internal
method will null-ify the dom/net field in any virError struct still
refering to them.
- In the remote daemon the paradigm for the helpers is usually
if (virDomainCreate (dom) == -1) {
virDomainFree(dom);
return -1;
}
virDomainFree(dom);
return 0;
Both of those virDomainFree calls will release the last reference
and thus, even if virDomainCreate() set the 'dom' field in the
virError object it will now get nullified.
- The remote daemon code which serializes the error object runs *after*
that snippet I show above.
Thus in the *absence* of memory leaks in the remote daemon helper methods
it is *impossible* for the 'dom' or 'net' fields to ever be set in the
error object serialized on the wire.
So there *is* a memory leak in server_error() on the client end, but it
is impossible to trigger it unless there is also a memory leak in the
server end :-) Now the virDoaminDestroy() method helper does currently
have a memory leak, hence why you saw the corresponding leak on the client
side. Fixing the server side leak, hid the client side leak. Also, once
a leak occurs on the server end, it will live forever, so once one method
leaks, you'll see the client side leaks for all other methods which trigger
an error too.
So we *do* need the patch to the server_error() method - at very least
for old daemons which will set the dom/net fields in the errors they
return.
We also ought to fix the daemon so that it correctly captures and
serializes the error objects. This is a very non-trivial fix, since
the code serializing the errors is running far too late. We need to
capture the error much earlier, which will probably require quite a
large change.
So I vote for applying all Cole's patches which do indeed fix a number
of memory leaks. Fixing the daemon to correctly serialize errors with
a dom/net object can be done later unless someone has burning desire
to tackle it now
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:44 a.m.
On Tue, May 20, 2008 at 03:38:01PM +0100, Daniel P. Berrange wrote:
So I vote for applying all Cole's patches which do indeed fix a
number
of memory leaks. Fixing the daemon to correctly serialize errors with
a dom/net object can be done later unless someone has burning desire
to tackle it now
Those fields are nothing but trouble. Any chance we can make a
special exception and get rid of them in a way which causes code that
depends on them to compile-fail with a meaningful error message, and
yet doesn't break old running code linking to a newer library?
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
9:44 a.m.
"Richard W.M. Jones" <rjones(a)redhat.com> wrote:
On Tue, May 20, 2008 at 03:38:01PM +0100, Daniel P. Berrange wrote:
> So I vote for applying all Cole's patches which do indeed fix a number
> of memory leaks. Fixing the daemon to correctly serialize errors with
> a dom/net object can be done later unless someone has burning desire
> to tackle it now
Those fields are nothing but trouble. Any chance we can make a
special exception and get rid of them in a way which causes code that
depends on them to compile-fail with a meaningful error message, and
yet doesn't break old running code linking to a newer library?
Hi Rich.
Good idea.
As a matter of fact, with gcc, there is a way to do that
for variables. I'd seen this applied to functions, but not
to variables. From gcc's extend.texi:
@cindex @code{deprecated} attribute
The @code{deprecated} attribute results in a warning if the variable
is used anywhere in the source file. This is useful when identifying
variables that are expected to be removed in a future version of a
program. The warning also includes the location of the declaration
of the deprecated variable, to enable users to easily find further
information about why the variable is deprecated, or what they should
do instead. Note that the warning only occurs for uses:
@smallexample
extern int old_var __attribute__ ((deprecated));
extern int old_var;
int new_fn () @{ return old_var; @}
@end smallexample
So I tried it and it works like a charm:
$ cat k.c
struct foo {
int ii;
int jj __attribute__((deprecated));
};
static int
bar()
{
struct foo o;
o.jj = 1;
};
$ gcc -c k.c
k.c: In function 'bar':
k.c:9: warning: 'jj' is deprecated (declared at k.c:3)
4:14 p.m.
On Mon, May 19, 2008 at 04:58:38PM -0400, Cole Robinson wrote:
Cole Robinson wrote:
> Domain and Net objects were not being cleaned up properly
> when reporting errors from the remote driver. Attached patch
> fixes this.
>
Stupid typo. Correct patch attached.
I've applied this to CVS now.
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
6062
days inactive
6065
days old
13 comments
4 participants
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Jim Meyering -
Richard W.M. Jones