[libvirt] [PATCH] virDrvStorageVolLookupByKey and virDrvStorageVolLookupByPath should use virStoragePoolPtr as parameter

Hello, These two functions, virDrvStorageVolLookupByKey and virDrvStorageVolLookupByPath should use virStoragePoolPtr as parameter instead of virConnectPtr for some few reasons: 1) Should follow the standard virStorage*Ptr parameters like the rest of storage related functions. 2) Functions now are able to access pool structure. This is particularly important for the optimization of the PowerHypervisor phypVolumeLookupByKey function. Thanks, -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com -- diff --git a/src/driver.h b/src/driver.h index 0975b59..bb05306 100644 --- a/src/driver.h +++ b/src/driver.h @@ -798,10 +798,10 @@ typedef virStorageVolPtr (*virDrvStorageVolLookupByName) (virStoragePoolPtr pool, const char *name); typedef virStorageVolPtr - (*virDrvStorageVolLookupByKey) (virConnectPtr pool, + (*virDrvStorageVolLookupByKey) (virStoragePoolPtr pool, const char *key); typedef virStorageVolPtr - (*virDrvStorageVolLookupByPath) (virConnectPtr pool, + (*virDrvStorageVolLookupByPath) (virStoragePoolPtr pool, const char *path); diff --git a/src/libvirt.c b/src/libvirt.c index 9d42c76..c43ce9c 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -8418,11 +8418,12 @@ error: * Returns a storage volume, or NULL if not found / error */ virStorageVolPtr -virStorageVolLookupByKey(virConnectPtr conn, +virStorageVolLookupByKey(virStoragePoolPtr pool, const char *key) { - DEBUG("conn=%p, key=%s", conn, key); + DEBUG("pool=%p, key=%s", pool, key); + virConnectPtr conn = pool->conn; virResetLastError(); if (!VIR_IS_CONNECT(conn)) { @@ -8437,7 +8438,7 @@ virStorageVolLookupByKey(virConnectPtr conn, if (conn->storageDriver && conn->storageDriver->volLookupByKey) { virStorageVolPtr ret; - ret = conn->storageDriver->volLookupByKey (conn, key); + ret = conn->storageDriver->volLookupByKey (pool, key); if (!ret) goto error; return ret; @@ -8461,11 +8462,12 @@ error: * Returns a storage volume, or NULL if not found / error */ virStorageVolPtr -virStorageVolLookupByPath(virConnectPtr conn, +virStorageVolLookupByPath(virStoragePoolPtr pool, const char *path) { - DEBUG("conn=%p, path=%s", conn, path); + DEBUG("pool=%p, path=%s", pool, path); + virConnectPtr conn = pool->conn; virResetLastError(); if (!VIR_IS_CONNECT(conn)) { @@ -8480,7 +8482,7 @@ virStorageVolLookupByPath(virConnectPtr conn, if (conn->storageDriver && conn->storageDriver->volLookupByPath) { virStorageVolPtr ret; - ret = conn->storageDriver->volLookupByPath (conn, path); + ret = conn->storageDriver->volLookupByPath (pool, path); if (!ret) goto error; return ret; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 80977a3..12380f4 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5559,9 +5559,10 @@ done: } static virStorageVolPtr -remoteStorageVolLookupByKey (virConnectPtr conn, +remoteStorageVolLookupByKey (virStoragePoolPtr pool, const char *key) { + virConnectPtr conn = pool->conn; virStorageVolPtr vol = NULL; remote_storage_vol_lookup_by_key_args args; remote_storage_vol_lookup_by_key_ret ret; @@ -5586,9 +5587,10 @@ done: } static virStorageVolPtr -remoteStorageVolLookupByPath (virConnectPtr conn, +remoteStorageVolLookupByPath (virStoragePoolPtr pool, const char *path) { + virConnectPtr conn = pool->conn; virStorageVolPtr vol = NULL; remote_storage_vol_lookup_by_path_args args; remote_storage_vol_lookup_by_path_ret ret; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index b148e39..25fe1d1 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1168,8 +1168,9 @@ cleanup: static virStorageVolPtr -storageVolumeLookupByKey(virConnectPtr conn, +storageVolumeLookupByKey(virStoragePoolPtr pool, const char *key) { + virConnectPtr conn = pool->conn; virStorageDriverStatePtr driver = conn->storagePrivateData; unsigned int i; virStorageVolPtr ret = NULL; @@ -1199,8 +1200,9 @@ storageVolumeLookupByKey(virConnectPtr conn, } static virStorageVolPtr -storageVolumeLookupByPath(virConnectPtr conn, +storageVolumeLookupByPath(virStoragePoolPtr pool, const char *path) { + virConnectPtr conn = pool->conn; virStorageDriverStatePtr driver = conn->storagePrivateData; unsigned int i; virStorageVolPtr ret = NULL; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 395c8c9..c65e4a0 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4251,8 +4251,9 @@ cleanup: static virStorageVolPtr -testStorageVolumeLookupByKey(virConnectPtr conn, +testStorageVolumeLookupByKey(virStoragePoolPtr pool, const char *key) { + virConnectPtr conn = pool->conn; testConnPtr privconn = conn->privateData; unsigned int i; virStorageVolPtr ret = NULL; @@ -4285,8 +4286,9 @@ testStorageVolumeLookupByKey(virConnectPtr conn, } static virStorageVolPtr -testStorageVolumeLookupByPath(virConnectPtr conn, +testStorageVolumeLookupByPath(virStoragePoolPtr pool, const char *path) { + virConnectPtr conn = pool->conn; testConnPtr privconn = conn->privateData; unsigned int i; virStorageVolPtr ret = NULL; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 6a9a2bf..ce665ef 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -7547,7 +7547,8 @@ static virStorageVolPtr vboxStorageVolLookupByName(virStoragePoolPtr pool, const return ret; } -static virStorageVolPtr vboxStorageVolLookupByKey(virConnectPtr conn, const char *key) { +static virStorageVolPtr vboxStorageVolLookupByKey(virStoragePoolPtr pool, const char *key) { + virConnectPtr conn = pool->conn; VBOX_OBJECT_CHECK(conn, virStorageVolPtr, NULL); vboxIID *hddIID = NULL; IHardDisk *hardDisk = NULL; @@ -7616,7 +7617,8 @@ cleanup: return ret; } -static virStorageVolPtr vboxStorageVolLookupByPath(virConnectPtr conn, const char *path) { +static virStorageVolPtr vboxStorageVolLookupByPath(virStoragePoolPtr pool, const char *path) { + virConnectPtr conn = pool->conn; VBOX_OBJECT_CHECK(conn, virStorageVolPtr, NULL); PRUnichar *hddPathUtf16 = NULL; IHardDisk *hardDisk = NULL;

On Mon, May 24, 2010 at 08:31:27PM -0300, Eduardo Otubo wrote:
Hello,
These two functions, virDrvStorageVolLookupByKey and virDrvStorageVolLookupByPath should use virStoragePoolPtr as parameter instead of virConnectPtr for some few reasons:
1) Should follow the standard virStorage*Ptr parameters like the rest of storage related functions.
Nope, this is not correct. Both 'path' and 'key' are intended a unique identifiers that are independant of a single pool. ie if you have a path /some/long/path/disk.img virDrvStorageVolLookupByPath() allows you to find the associated virStorageVolPtr object without having to know what pool it is within. Similarly 'key' is intended as a globally unique ID, for convenience most impls currently just reuse path value for the key too, but this isn't required & can even be bad. For an NFS server for example, the key should really be a combination of the host name + export path + relative path within the export, instead of the local client path. This ensures the key is unique + stable even if multiple clients mount the same volume in different places. Furthermore changing the parameters in these functions would break the API compatability wrt previous libvirt, which is not allowed. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.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)
-
Daniel P. Berrange
-
Eduardo Otubo