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

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?