On Sat, Jan 19, 2008 at 07:42:35PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 -=|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list