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);

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?

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);

/* 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);

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,

Daniel P. Berrange wrote: Oops, of course. I've fixed this up and committed the result; the final patch is attached. Thanks for the review, -- Chris Lalancette

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 3 Nov 2008 11:32:15 -0000 @@ -357,16 +357,17 @@ char * virStorageBackendStablePath(virConnectPtr conn, virStoragePoolObjPtr pool, - char *devpath) + const char *devpath) { DIR *dh; struct dirent *dent; + char *stablepath; /* Short circuit if pool has no target, or if its /dev */ if (pool->def->target.path == NULL || STREQ(pool->def->target.path, "/dev") || STREQ(pool->def->target.path, "/dev/")) - return devpath; + goto ret_strdup; /* The pool is pointing somewhere like /dev/disk/by-path * or /dev/disk/by-id, so we need to check all symlinks in @@ -382,7 +383,6 @@ } while ((dent = readdir(dh)) != NULL) { - char *stablepath; if (dent->d_name[0] == '.') continue; @@ -407,10 +407,17 @@ closedir(dh); + ret_strdup: /* Couldn't find any matching stable link so give back * the original non-stable dev path */ - return devpath; + + stablepath = strdup(devpath); + + if (stablepath == NULL) + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("dup path"));