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