[libvirt] [PATCH]: Allow arbitrary paths to virStorageVolLookupByPath

[Daniel P. Berrange]

On Fri, Oct 31, 2008 at 12:58:17PM +0100, Chris Lalancette wrote:
> Daniel P. Berrange wrote:
> >> Personally, I think those are bad semantics for virStorageBackendStablePath;
> >> assuming it succeeds, you should always be able to know that you have a copy,
> >> regardless of whether the copy is the same as the original.  Should I change
> >> virStorageBackendStablePath to those semantics, in which case your below code
> >> would then be correct?
> > 
> > Yes, I think that's worth doing - will also avoid the cast in the input
> > arg there
> 
> OK, updated patch attached; virStorageBackendStablePath now always returns a
> copy of the given string, so it's always safe to unconditionally VIR_FREE it.  I
> fixed up storage_backend_iscsi and storage_backend_disk to reflect this change.
>  I also re-worked the code as you suggested, and added a bit more error checking.
> 

> Index: src/storage_backend.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/storage_backend.c,v
> retrieving revision 1.24
> diff -u -r1.24 storage_backend.c
> --- src/storage_backend.c	28 Oct 2008 17:48:06 -0000	1.24
> +++ src/storage_backend.c	31 Oct 2008 11:56:33 -0000
> @@ -357,7 +357,7 @@
>  char *
>  virStorageBackendStablePath(virConnectPtr conn,
>                              virStoragePoolObjPtr pool,
> -                            char *devpath)
> +                            const char *devpath)
>  {
>      DIR *dh;
>      struct dirent *dent;
> @@ -366,7 +366,7 @@
>      if (pool->def->target.path == NULL ||
>          STREQ(pool->def->target.path, "/dev") ||
>          STREQ(pool->def->target.path, "/dev/"))
> -        return devpath;
> +        return strdup(devpath);

Need to call virStorageReportError here on OOM.

>  
>      /* The pool is pointing somewhere like /dev/disk/by-path
>       * or /dev/disk/by-id, so we need to check all symlinks in
> @@ -410,7 +410,7 @@
>      /* Couldn't find any matching stable link so give back
>       * the original non-stable dev path
>       */
> -    return devpath;
> +    return strdup(devpath);

And here.

Since virStorageBackendStablePath() api contract says that it is responsible
for setting the errors upon failure.

> Index: src/storage_driver.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/storage_driver.c,v
> retrieving revision 1.13
> diff -u -r1.13 storage_driver.c
> --- src/storage_driver.c	21 Oct 2008 17:15:53 -0000	1.13
> +++ src/storage_driver.c	31 Oct 2008 11:56:34 -0000
> @@ -966,8 +966,34 @@
>  
>      for (i = 0 ; i < driver->pools.count ; i++) {
>          if (virStoragePoolObjIsActive(driver->pools.objs[i])) {
> -            virStorageVolDefPtr vol =
> -                virStorageVolDefFindByPath(driver->pools.objs[i], path);
> +            virStorageVolDefPtr vol;
> +            virStorageBackendPoolOptionsPtr options;
> +
> +            options = virStorageBackendPoolOptionsForType(driver->pools.objs[i]->def->type);
> +            if (options == NULL)
> +                continue;
> +
> +            if (options->flags & VIR_STORAGE_BACKEND_POOL_STABLE_PATH) {
> +                const char *stable_path;
> +
> +                stable_path = virStorageBackendStablePath(conn,
> +                                                          driver->pools.objs[i],
> +                                                          path);
> +                /*
> +                 * virStorageBackendStablePath already does
> +                 * virStorageReportError if it fails; we just need to keep
> +                 * propagating the return code
> +                 */
> +                if (stable_path == NULL)
> +                    return NULL;
> +
> +                vol = virStorageVolDefFindByPath(driver->pools.objs[i],
> +                                                 stable_path);
> +                VIR_FREE(stable_path);
> +            }
> +            else
> +                vol = virStorageVolDefFindByPath(driver->pools.objs[i], path);
> +
>  
>              if (vol)
>                  return virGetStorageVol(conn,

This looks good now.


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|