[Libvir] Repository for work-in-progress storage patches

[Daniel P. Berrange]

On Sat, Jan 19, 2008 at 07:42:35PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange at 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  -=|