[libvirt] using public vir*Free() (instead of virUnref*()) from vir*FreeName() destructors.

[Daniel P. Berrange]

On Thu, May 07, 2009 at 12:22:56PM -0400, Laine Stump wrote:
> I just noticed in datatypes.c that the 
> vir(Network|StoragePool|StorageVol)FreeName destructors call the public 
> API vir*Free() functions rather than the local virUnref*(). vir*Free() 
> all clear any errors, which seems like it might not be the right thing 
> to do (eg, if we're cleaning out the hashtables as part of some 
> error-handling path, and haven't yet logged the error).
> 
> On the other hand, the public API function does some sanity checking on 
> the object to make sure it really is what we think it is before calling 
> the unref. So it's got that going for it. (5 points to anyone who 
> catches the reference without going to Google) But I don't know enough 
> to know whether or not that's important in this case.
> 
> So which is more important: the extra sanity check, or not clearing 
> existing error codes?

IMHO the existing code is wrong, because these functions are invokved
from the hash table destructor, which is invokved from the code that
destroys the virConnectPtr. It really isn't safe to call back to the
public APIs like this from the middle of the virCOnnectPtr destructor.
So we should change them to directly call Unref.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -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 :|