[libvirt] [PATCH] Allow relative path for qemu backing file

[Jesse J Cook]

On Mon, 2011-04-04 at 16:39 -0600, Eric Blake wrote:
> On 03/27/2011 07:30 PM, Jesse Cook wrote:
> > This patch enables the relative backing file path support provided by
> > qemu-img create.
> > 
> > If a relative path is specified for the backing file, it is converted
> > to an absolute path using the storage pool path. The absolute path is
> > used to verify that the backing file exists. If the backing file exists,
> > the relative path is allowed and will be provided to qemu-img create.
> > 
> > This patch takes the place of a previous patch:
> >   http://www.redhat.com/archives/libvir-list/2011-March/msg00179.html
> > ---
> >  src/storage/storage_backend.c |   18 +++++++++++++++++-
> >  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> Sorry for not reviewing this in time for 0.9.0.

No Problem. It took me a while to get around to the fix.

> > @@ -686,7 +689,20 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
> >                                    vol->backingStore.format);
> >              return -1;
> >          }
> > -        if (access(vol->backingStore.path, R_OK) != 0) {
> > +
> > +        /* Convert relative backing store paths to absolute paths for access
> > +         * validation.
> > +         */
> > +        if ('/' != *(vol->backingStore.path)) {
> > +            virAsprintf(&absolutePath, "%s/%s", pool->def->target.path,
> > +                        vol->backingStore.path);
> > +
> > +        } else {
> > +            virAsprintf(&absolutePath, "%s", vol->backingStore.path);
> 
> strdup is more efficient here, and avoiding malloc in the first place
> even more so.
> 
> > +        }
> > +        accessRetCode = access(absolutePath, R_OK);
> 
> This could segfault on OOM.
> 
> > +        VIR_FREE(absolutePath);

I believe there needs to be a NULL check here or absolute paths and
virAsprintf errors will segfault. I can patch if you don't beat me to
it.

> > +        if (accessRetCode != 0) {
> >              virReportSystemError(errno,
> >                                   _("inaccessible backing store volume %s"),
> >                                   vol->backingStore.path);
> 
> ACK with nits fixed; so here's what I squashed in before pushing.  I
> also updated AUTHORS to pass 'make syntax-check'; feel free to let me
> know off-list if you prefer any alternate spelling there (especially
> since you sent v1 and v2 under different email aliases).
> 
> diff --git i/src/storage/storage_backend.c w/src/storage/storage_backend.c
> index 5f5565b..8af2878 100644
> --- i/src/storage/storage_backend.c
> +++ w/src/storage/storage_backend.c
> @@ -693,7 +693,6 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
> 
>      if (vol->backingStore.path) {
>          int accessRetCode = -1;
> -
>          char *absolutePath = NULL;
> 
>          /* XXX: Not strictly required: qemu-img has an option a different
> @@ -719,14 +718,14 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
>          /* Convert relative backing store paths to absolute paths for
> access
>           * validation.
>           */
> -        if ('/' != *(vol->backingStore.path)) {
> +        if ('/' != *(vol->backingStore.path) &&
>              virAsprintf(&absolutePath, "%s/%s", pool->def->target.path,
> -                        vol->backingStore.path);
> -
> -        } else {
> -            virAsprintf(&absolutePath, "%s", vol->backingStore.path);
> +                        vol->backingStore.path) < 0) {
> +            virReportOOMError();
> +            return -1;
>          }
> -        accessRetCode = access(absolutePath, R_OK);
> +        accessRetCode = access(absolutePath ? absolutePath
> +                               : vol->backingStore.path, R_OK);
>          VIR_FREE(absolutePath);
>          if (accessRetCode != 0) {
>              virReportSystemError(errno,
> 
> 

-- 
Jesse Cook
Research Scientist
EADS NA Defense Security & Systems Solutions, Inc. (DS3)
Email: jesse.cook at eads-na-security.com