[libvirt] virStoragePoolSourceFree(S) does not free S

[Jim Meyering]

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.