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

[Daniel P. Berrange]

On Fri, Oct 31, 2008 at 12:26:17PM +0100, Chris Lalancette wrote:
> Daniel P. Berrange wrote:
> > On Fri, Oct 31, 2008 at 12:04:34PM +0100, Chris Lalancette wrote:
> >> 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
> >> --- a/src/storage_driver.c	21 Oct 2008 17:15:53 -0000	1.13
> >> +++ b/src/storage_driver.c	31 Oct 2008 10:09:29 -0000
> >> @@ -963,11 +963,25 @@
> >>      virStorageDriverStatePtr driver =
> >>          (virStorageDriverStatePtr)conn->storagePrivateData;
> >>      unsigned int i;
> >> +    char *stable_path;
> >>  
> >>      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)
> >> +                stable_path = virStorageBackendStablePath(conn, driver->pools.objs[i], (char *)path);
> >> +            else
> >> +                stable_path = (char *)path;
> >> +
> >> +            vol =  virStorageVolDefFindByPath(driver->pools.objs[i], stable_path);
> >> +            if (stable_path != path)
> >> +                VIR_FREE(stable_path);
> > 
> > This VIR_FREE check is slightly evil, how about something more like
> 
> Well, I originally stole this pointer comparison from storage_backend_iscsi.c
> which does the same thing.  While I agree that the code below is more clear, it
> actually has a subtle bug; if you pass in, say "/dev/sdc", and the pool target
> path is of type "/dev", then virStorageBackendStablePath() will return the
> original pointer, not a copy.  So in the below code, you would actually end up
> freeing path, not a copy of path.
> 
> 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

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 :|