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.