[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Domain/Net object cleanups after remote error



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]