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?
if (options->flags & VIR_STORAGE_BACKEND_POOL_STABLE_PATH) {
char *stablepath = virStorageBackendStablePath(conn,
driver->pools.objs[i], path);
vol = virStorageVolDefFindByPath(driver->pools.objs[i],
stable_path);
VIR_FREE(stablepath);
} else {
vol = virStorageVolDefFindByPath(driver->pools.objs[i], path);
}
--
Chris Lalancette