On Sat, Jan 19, 2008 at 07:42:35PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Sat, Jan 19, 2008 at 07:09:31PM +0100, Jim Meyering wrote: ...
In storage_backend_loop.c, it looks like vol->target.path can be leaked.
Which function is that in ? Since originally writing it I've changed all error path cleanup code to simply call virStorageVolDefFree(), so there's a central function which will free all members, rather than having cleanup duplicated all over the place.
This is the state from yesterday:
static int virStorageBackendLoopVolCreate(virConnectPtr conn, ... if ((vol->target.path = malloc(5 + strlen(vol->name) + 1)) == NULL) { virStorageReportError(conn, VIR_ERR_NO_MEMORY, "target"); return -1; } strcpy(vol->target.path, "/dev/"); strcat(vol->target.path, vol->name);
if ((vol->key = strdup(vol->target.path)) == NULL) { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "storage vol key"); return -1; }
if (virRun(conn, (char**)cmdargv, NULL) < 0) return -1;
if ((fd = open64(vol->target.path, O_RDONLY)) < 0) { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "cannot read path '%s': %d (%s)", vol->target.path, errno, strerror(errno)); goto cleanup; } ... cleanup: close(fd); virStorageBackendLoopVolDelete(conn, pool, vol); return -1; }
----------------------------- I'd say just do s/return -1/goto cleanup/ after failed strdup but then you have to be careful to set fd = -1, and avoid calling "close" on it in that case.
Actually that's OK - the contract of the 'volCreate' method in the backend does not require free'ing of the 'vol' object upon failure. Becaue 'vol' is passed in pre-allocated, it is the caller's responsibilty to release the 'vol' object upon failure. So the cleanup code in the loop driver only needs to cleanup mess it made itself - eg releasing the loop device and closing the fd. Take a look at the storageVolumeCreateXML() method in storage_driver.c + if (backend->createVol(obj->conn, pool, vol) < 0) { + virStorageVolDefFree(vol); + return NULL; + } That 'createVol' call is what's invoking virStorageBackendLoopVolCreate() I should document this API contract alongside the driver API to make this clear. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|