Daniel P. Berrange wrote:
> 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.
That would avoid confusion (and error!). Of course, such
clean-up changes should be separate from bug-fixing ones.
Since you didn't object to the leak-plugging patch that started this,
I'll push it shortly.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list