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