Re: [libvirt] virStoragePoolSourceFree(S) does not free S

On Fri, Feb 05, 2010 at 05:26:35PM +0100, Jim Meyering wrote: > I was surprised to see that virStoragePoolSourceFree(S) does not free S. > The other three vir*Free functions in storage_conf *do* free S. [snip] > One fix might be to call VIR_FREE(def) in the "if (def)..." > block, but that seems short-sighted, since the name > virStoragePoolSourceFree makes me think *it* should be > doing the whole job. It is a bad name - it should be renamed to virStoragePoolSourceClear() > > However, if I make the logical change to virStoragePoolSourceFree, > > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index 62b8394..ffe38cc 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -291,6 +291,7 @@ virStoragePoolSourceFree(virStoragePoolSourcePtr source) { > VIR_FREE(source->auth.chap.login); > VIR_FREE(source->auth.chap.passwd); > } > + VIR_FREE(source); > } > > that causes "make check" to fail miserably due to heap corruption, > as reported by valgrind: > I tracked the problem down to this definition in src/conf/storage_conf.h: > > typedef struct _virStoragePoolDef virStoragePoolDef; > typedef virStoragePoolDef *virStoragePoolDefPtr; > struct _virStoragePoolDef { > /* General metadata */ > char *name; > unsigned char uuid[VIR_UUID_BUFLEN]; > int type; /* virStoragePoolType */ > > unsigned long long allocation; > unsigned long long capacity; > unsigned long long available; > > virStoragePoolSource source; <== this is a *STRUCT*, not a ptr > virStoragePoolTarget target; <== Likewise > }; Yep, the 'virStoragePoolSource' object is embedded directly in other structs, not referenced via a pointer, so you can't free this object directly most of the time... except we later added an internal API which lets you use virStoragePoolSource as a standalone object which does need free'ing. I think we need to rename the current virStoragePoolSourceFree to virStoragePoolSourceClear(), and then add a new implmenetation of virStoragePoolSourceFree that calls Clear() and VIR_FREE(def) making the latter be used where applicable.