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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list