[libvirt] [PATCH]: Allow arbitrary paths to virStorageVolLookupByPath
In ovirt, we have to scan iSCSI LUN's for LVM storage when they are first added to the database. To do this, we do roughly the following: iscsi_pool = libvirt.define_and_start_iscsi_pool("/dev/disk/by-id") iscsi_pool.add_luns_to_db logical = conn.discover_storage_pool_sources("logical") for each logical_volume_group in logical: for each device in logical_volume_group: iscsi_pool.lookup_vol_by_path(device.path) And then we use that information to associate an iSCSI LUN with this volume group. The problem is that there is an mis-match between how we define the iscsi pool (with /dev/disk/by-id devices), and what data the discover_storage_pool_sources() returns (/dev devices), so we can't easily associate the two. The following patch implements stable path naming when the virStorageVolLookupByPath method is called. Basically, it tries to convert whatever path it is given (say /dev/sdc) into the form currently used by the Pool (say /dev/disk/by-id). It then goes and looks up the form in the pool, and returns the storageVolume object as appropriate. Note that it only tries to do this for the Pool types where it makes sense, namely iSCSI and disk; all other pool types stay exactly the same. With this in place, we can solve the mis-match in the above code, and LVM scanning seems to work in ovirt. Signed-off-by: Chris Lalancette <clalance@redhat.com>
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 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); } 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 :|
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
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 :|
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. Signed-off-by: Chris Lalancette <clalance@redhat.com>
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 :|
Daniel P. Berrange wrote:
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.
Oops, of course. I've fixed this up and committed the result; the final patch is attached. Thanks for the review, -- Chris Lalancette
On Mon, Nov 03, 2008 at 12:38:49PM +0100, Chris Lalancette wrote:
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"));
Don't bother with passing a message with any VIR_ERR_NO_MEMORY errors - just use NULL. The message is totally ignored for this error code, and 'dup path' is useless info for the end user anyway 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 :|
participants (2)
-
Chris Lalancette -
Daniel P. Berrange