5 a.m.
On Fri, Mar 28, 2008 at 04:39:31PM +0100, Guido Guenther wrote:
---
src/virterror.c | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/src/virterror.c b/src/virterror.c
index 1463129..49f3b89 100644
--- a/src/virterror.c
+++ b/src/virterror.c
@@ -115,6 +115,11 @@ virResetError(virErrorPtr err)
free(err->str1);
free(err->str2);
free(err->str3);
+ if(err->dom) {
+ if(err->dom->name)
+ free(err->dom->name);
+ free(err->dom);
+ }
memset(err, 0, sizeof(virError));
}
@@ -383,6 +388,15 @@ __virRaiseError(virConnectPtr conn, virDomainPtr dom, virNetworkPtr
net,
virResetError(to);
to->conn = conn;
to->dom = dom;
+ if (dom) {
+ if (!(to->dom = malloc(sizeof(virDomain)))) {
+ fprintf(stderr, "cannot allocate memory in %s", __FUNCTION__);
+ } else {
+ to->dom->name = strdup(dom->name);
+ memcpy (to->dom->uuid, dom->uuid, VIR_UUID_BUFLEN);
+ to->dom->id = dom->id;
+ }
+ }
to->net = net;
to->domain = domain;
to->code = code;
What problem are you trying to solve?
The 'conn', 'dom' and 'net' fields in a virterror can't be
accessed
safely, particularly from a garbage-collected language. The only safe
thing one could do with them is a physical equality test on the
pointer against an existing conn/dom/net pointer.
But the solution isn't to copy the dom object, since in a GC-d
language someone could grab a handle to the copied dom which would
have a different lifetime from the error object. If anything the
solution would be to remove those fields from the error object since
they are highly unlikely to be useful in any real world application.
We can't do this because of backwards compatibility promises but we
should instead deprecate them.
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.
New subject: [Libvir] [PATCH] deep copy dom structure
On Mon, Mar 31, 2008 at 11:00:56AM +0100, Richard W.M. Jones wrote:
What problem are you trying to solve?
AFAICT, the problem is that the Domain/Network objects are being free'd
before the remote daemon has a chance to serialze the Error object into
XDR format. So it is accessing free'd memory when serializing the dom/net
field in the Error object & geting back garbage.
The 'conn', 'dom' and 'net' fields in a
virterror can't be accessed
safely, particularly from a garbage-collected language. The only safe
thing one could do with them is a physical equality test on the
pointer against an existing conn/dom/net pointer.
But the solution isn't to copy the dom object, since in a GC-d
language someone could grab a handle to the copied dom which would
have a different lifetime from the error object. If anything the
solution would be to remove those fields from the error object since
they are highly unlikely to be useful in any real world application.
We can't do this because of backwards compatibility promises but we
should instead deprecate them.
We should at the very least NULL-ify the dom/net fields in the Error
object associated with the Connection when we free the Domain/Network
object. We also probably need to re-arrange the remote daemon code a
little so that it serializes to XDR format before the Domain/Network object
are free'd.
I agree that deep copying isn't the answer here & messes up the reference
counting and will cause potential memory leaks.
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 :|
7:12 a.m.
New subject: [Libvir] [PATCH] deep copy dom structure
On Mon, Mar 31, 2008 at 01:02:49PM +0100, Daniel P. Berrange wrote:
On Mon, Mar 31, 2008 at 11:00:56AM +0100, Richard W.M. Jones wrote:
> The 'conn', 'dom' and 'net' fields in a virterror can't
be accessed
> safely, particularly from a garbage-collected language. The only safe
> thing one could do with them is a physical equality test on the
> pointer against an existing conn/dom/net pointer.
>
> But the solution isn't to copy the dom object, since in a GC-d
> language someone could grab a handle to the copied dom which would
> have a different lifetime from the error object. If anything the
> solution would be to remove those fields from the error object since
> they are highly unlikely to be useful in any real world application.
> We can't do this because of backwards compatibility promises but we
> should instead deprecate them.
We should at the very least NULL-ify the dom/net fields in the Error
object associated with the Connection when we free the Domain/Network
object.
agreed, very simple test but avoids dandling pointers.
We also probably need to re-arrange the remote daemon code a
little so that it serializes to XDR format before the Domain/Network object
are free'd.
I agree that deep copying isn't the answer here & messes up the reference
counting and will cause potential memory leaks.
yes, this will become unmanageable very fast i'm afraid.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
7:49 a.m.
New subject: [Libvir] [PATCH] deep copy dom structure
On Mon, Mar 31, 2008 at 08:12:19AM -0400, Daniel Veillard wrote:
On Mon, Mar 31, 2008 at 01:02:49PM +0100, Daniel P. Berrange wrote:
> We should at the very least NULL-ify the dom/net fields in the Error
> object associated with the Connection when we free the Domain/Network
> object.
agreed, very simple test but avoids dandling pointers.
My patch for this. it's a bit more complex because we also keep a
global variable for the last variable, and that needed to be exported to
the hash module, it's still rather simple,
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
8:35 a.m.
New subject: [Libvir] [PATCH] deep copy dom structure
On Mon, Mar 31, 2008 at 08:49:12AM -0400, Daniel Veillard wrote:
On Mon, Mar 31, 2008 at 08:12:19AM -0400, Daniel Veillard wrote:
> On Mon, Mar 31, 2008 at 01:02:49PM +0100, Daniel P. Berrange wrote:
> > We should at the very least NULL-ify the dom/net fields in the Error
> > object associated with the Connection when we free the Domain/Network
> > object.
>
> agreed, very simple test but avoids dandling pointers.
My patch for this. it's a bit more complex because we also keep a
global variable for the last variable, and that needed to be exported to
the hash module, it's still rather simple,
Yes, that's pretty much what I was thinking we should do - ACK from me.
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:38 a.m.
New subject: [Libvir] [PATCH] deep copy dom structure
On Mon, Mar 31, 2008 at 02:35:31PM +0100, Daniel P. Berrange wrote:
On Mon, Mar 31, 2008 at 08:49:12AM -0400, Daniel Veillard wrote:
> On Mon, Mar 31, 2008 at 08:12:19AM -0400, Daniel Veillard wrote:
> > On Mon, Mar 31, 2008 at 01:02:49PM +0100, Daniel P. Berrange wrote:
> > > We should at the very least NULL-ify the dom/net fields in the Error
> > > object associated with the Connection when we free the Domain/Network
> > > object.
> >
> > agreed, very simple test but avoids dandling pointers.
>
> My patch for this. it's a bit more complex because we also keep a
> global variable for the last variable, and that needed to be exported to
> the hash module, it's still rather simple,
Yes, that's pretty much what I was thinking we should do - ACK from me.
thanks,
commited to CVS,
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
8:40 a.m.
New subject: [Libvir] [PATCH] deep copy dom structure
On Mon, Mar 31, 2008 at 08:49:12AM -0400, Daniel Veillard wrote:
On Mon, Mar 31, 2008 at 08:12:19AM -0400, Daniel Veillard wrote:
> On Mon, Mar 31, 2008 at 01:02:49PM +0100, Daniel P. Berrange wrote:
> > We should at the very least NULL-ify the dom/net fields in the Error
> > object associated with the Connection when we free the Domain/Network
> > object.
>
> agreed, very simple test but avoids dandling pointers.
My patch for this. it's a bit more complex because we also keep a
global variable for the last variable, and that needed to be exported to
the hash module, it's still rather simple,
Yes, ack from me too.
I'm still going to deprecate these in the OCaml bindings, or maybe
just change them so you can only do physical pointer comparisons. I
just can't see a real life use for them ... You already know which
call failed and from that what domain it was operating on, so except
perhaps in the case of migration there is no new information here.
Also I've not seen any libvirt program yet which does more than just
print the error message and either abort or ignore it.
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
9:39 a.m.
New subject: [Libvir] [PATCH] deep copy dom structure
On Mon, Mar 31, 2008 at 02:40:54PM +0100, Richard W.M. Jones wrote:
On Mon, Mar 31, 2008 at 08:49:12AM -0400, Daniel Veillard wrote:
> On Mon, Mar 31, 2008 at 08:12:19AM -0400, Daniel Veillard wrote:
> > On Mon, Mar 31, 2008 at 01:02:49PM +0100, Daniel P. Berrange wrote:
> > > We should at the very least NULL-ify the dom/net fields in the Error
> > > object associated with the Connection when we free the Domain/Network
> > > object.
> >
> > agreed, very simple test but avoids dandling pointers.
>
> My patch for this. it's a bit more complex because we also keep a
> global variable for the last variable, and that needed to be exported to
> the hash module, it's still rather simple,
Yes, ack from me too.
I'm still going to deprecate these in the OCaml bindings, or maybe
just change them so you can only do physical pointer comparisons. I
just can't see a real life use for them ... You already know which
call failed and from that what domain it was operating on, so except
perhaps in the case of migration there is no new information here.
Also I've not seen any libvirt program yet which does more than just
print the error message and either abort or ignore it.
I just looked at the python binding, and it doesn't even bother to copy
the dom/net fields from virErrorPtr into the python layer - just completely
ignores them. Similarly I don't bother to do anything with them in the Perl
bindings. IMHO, we should just mark them as deprecated in the API docs &
tell people not to use them. Note, we don't even have the virStoragePoolPtr
or virStorageVolPtr objects in the error either.
Regards,
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:45 a.m.
New subject: [Libvir] [PATCH] deep copy dom structure
On Mon, Mar 31, 2008 at 03:39:06PM +0100, Daniel P. Berrange wrote:
bindings. IMHO, we should just mark them as deprecated in the API
docs &
tell people not to use them. Note, we don't even have the virStoragePoolPtr
or virStorageVolPtr objects in the error either.
Patch attached.
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
9:51 a.m.
New subject: [Libvir] [PATCH] deep copy dom structure
On Mon, Mar 31, 2008 at 03:45:37PM +0100, Richard W.M. Jones wrote:
On Mon, Mar 31, 2008 at 03:39:06PM +0100, Daniel P. Berrange wrote:
> bindings. IMHO, we should just mark them as deprecated in the API docs &
> tell people not to use them. Note, we don't even have the virStoragePoolPtr
> or virStorageVolPtr objects in the error either.
Patch attached.
There i disagree, maybe the bindings don't use them but it could be
used perfectly fine if you process the error directly in the callback
from the library, i.e. in a synchronous way. I would not flag them as
deprecated, just that one need to be careful to use them asynchronously.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
12:13 p.m.
New subject: [Libvir] [PATCH] deep copy dom structure
The 'conn', 'dom' and 'net' fields in a
virterror can't be accessed
safely, particularly from a garbage-collected language. The only safe
thing one could do with them is a physical equality test on the
pointer against an existing conn/dom/net pointer.
The problem is that we're freeing the dom/net objects before serializing
the result so we're accessing already freed memory (which only works by
accident at the momemt). I'll have a look at moving around virDomainFree()
and friends when back from vactions.
-- Guido
6112
days inactive
6112
days old
10 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
guido＠honk.sigxcpu.org -
Richard W.M. Jones