[libvirt] [PATCH v2 0/2] small gluster fixes
Hi, I noticed that "virsh pool-list --type gluster" is not giving me an empty list as expected, but rather the list of active pools. This is happening because virConnectListAllStoragePools() is returning VIR_ERROR_INVALID_ARG because VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER is unrecognized. vshStoragePoolListCollect then goes to retry with only VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE | VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE in the flags, which is questionable, but explains the result I got. Adding VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER to VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE is enough to fix that. I've played a bit with adding some compile time checks to avoid this in the future, but while they work, they are a bit ugly-looking, and they would require some changes in docs/apibuild.py to teach it to parse 1 << N, or to make it ignore values in #ifdef VIR_ENUM_SENTINELS, so before spending too much time on this, I'd like to know if these checks would be a welcome/useful addition. The checks are: commit 2aee2a1827497ebde19738c88d2ababfe2dbb13d Author: Christophe Fergeau <cfergeau@redhat.com> Date: Thu Feb 6 17:39:43 2014 +0100 add pool related compile-time checks diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index b3ce000..9d79ae6 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3104,6 +3104,10 @@ typedef enum { VIR_CONNECT_LIST_STORAGE_POOLS_RBD = 1 << 14, VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG = 1 << 15, VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER = 1 << 16, + +#ifdef VIR_ENUM_SENTINELS + VIR_CONNECT_LIST_STORAGE_POOLS_LAST +#endif } virConnectListAllStoragePoolsFlags; int virConnectListAllStoragePools(virConnectPtr conn, diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index eaa9325..c8d4df4 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -36,6 +36,7 @@ #include "virerror.h" #include "datatypes.h" #include "storage_conf.h" +#include "verify.h" #include "virstoragefile.h" #include "virxml.h" @@ -51,6 +52,16 @@ #define DEFAULT_POOL_PERM_MODE 0755 #define DEFAULT_VOL_PERM_MODE 0600 +/* Check if VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE seems to + * contain all virConnectListAllStoragePoolsFlags elements + */ +verify(VIR_CONNECT_LIST_STORAGE_POOLS_LAST < VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE); + +/* Check if virConnectListAllStorageFlags has one flag per known storage + * pool type */ +verify(1 << (5+VIR_STORAGE_POOL_LAST) == VIR_CONNECT_LIST_STORAGE_POOLS_LAST - 1); + + VIR_ENUM_IMPL(virStorageVol, VIR_STORAGE_VOL_LAST, "file", "block", "dir", "network", "netdir") @@ -281,6 +292,8 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { } }; +/* Check if poolTypeInfo has one entry per known storage pool type */ +verify(ARRAY_CARDINALITY(poolTypeInfo) == VIR_STORAGE_POOL_LAST); static virStoragePoolTypeInfoPtr virStoragePoolTypeInfoLookup(int type) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
LIBGLUSTER_LIBS is emptied before gluster is enabled/disabled, but nothing else sets/uses this variable, so it can be removed. --- configure.ac | 1 - 1 file changed, 1 deletion(-) diff --git a/configure.ac b/configure.ac index 884e0e4..e233706 100644 --- a/configure.ac +++ b/configure.ac @@ -1907,7 +1907,6 @@ fi AM_CONDITIONAL([WITH_STORAGE_SHEEPDOG], [test "$with_storage_sheepdog" = "yes"]) -LIBGLUSTER_LIBS= if test "$with_storage_gluster" = "check"; then with_storage_gluster=$with_glusterfs fi -- 1.8.5.3
On 02/06/2014 11:09 AM, Christophe Fergeau wrote:
LIBGLUSTER_LIBS is emptied before gluster is enabled/disabled, but nothing else sets/uses this variable, so it can be removed. --- configure.ac | 1 - 1 file changed, 1 deletion(-)
diff --git a/configure.ac b/configure.ac index 884e0e4..e233706 100644 --- a/configure.ac +++ b/configure.ac @@ -1907,7 +1907,6 @@ fi AM_CONDITIONAL([WITH_STORAGE_SHEEPDOG], [test "$with_storage_sheepdog" = "yes"])
-LIBGLUSTER_LIBS= if test "$with_storage_gluster" = "check"; then
ACK. Leftovers from my earlier drafts. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
If it's not present in this list, we won't be able to get only glusterfs pools when using virConnectListAllStoragePools. --- src/conf/storage_conf.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 62ac749..cada861 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -575,7 +575,8 @@ VIR_ENUM_DECL(virStoragePartedFsType) VIR_CONNECT_LIST_STORAGE_POOLS_SCSI | \ VIR_CONNECT_LIST_STORAGE_POOLS_MPATH | \ VIR_CONNECT_LIST_STORAGE_POOLS_RBD | \ - VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG) + VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG | \ + VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER) # define VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ALL \ (VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE | \ -- 1.8.5.3
On 02/06/2014 11:09 AM, Christophe Fergeau wrote:
If it's not present in this list, we won't be able to get only glusterfs pools when using virConnectListAllStoragePools. --- src/conf/storage_conf.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 62ac749..cada861 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -575,7 +575,8 @@ VIR_ENUM_DECL(virStoragePartedFsType) VIR_CONNECT_LIST_STORAGE_POOLS_SCSI | \ VIR_CONNECT_LIST_STORAGE_POOLS_MPATH | \ VIR_CONNECT_LIST_STORAGE_POOLS_RBD | \ - VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG) + VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG | \ + VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/06/2014 11:09 AM, Christophe Fergeau wrote:
I've played a bit with adding some compile time checks to avoid this in the future, but while they work, they are a bit ugly-looking, and they would require some changes in docs/apibuild.py to teach it to parse 1 << N, or to make it ignore values in #ifdef VIR_ENUM_SENTINELS, so before spending too much time on this, I'd like to know if these checks would be a welcome/useful addition.
Hmm, we don't add pools every day.
+/* Check if VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE seems to + * contain all virConnectListAllStoragePoolsFlags elements + */ +verify(VIR_CONNECT_LIST_STORAGE_POOLS_LAST < VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE); + +/* Check if virConnectListAllStorageFlags has one flag per known storage + * pool type */ +verify(1 << (5+VIR_STORAGE_POOL_LAST) == VIR_CONNECT_LIST_STORAGE_POOLS_LAST - 1);
Wow, that does look pretty ugly and hard-coded. At this point, I'm not sure that adding it buys us much. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Feb 06, 2014 at 01:56:17PM -0700, Eric Blake wrote:
On 02/06/2014 11:09 AM, Christophe Fergeau wrote:
+/* Check if VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE seems to + * contain all virConnectListAllStoragePoolsFlags elements + */ +verify(VIR_CONNECT_LIST_STORAGE_POOLS_LAST < VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE); + +/* Check if virConnectListAllStorageFlags has one flag per known storage + * pool type */ +verify(1 << (5+VIR_STORAGE_POOL_LAST) == VIR_CONNECT_LIST_STORAGE_POOLS_LAST - 1);
Wow, that does look pretty ugly and hard-coded. At this point, I'm not sure that adding it buys us much.
Agreed about the ugliness ;) I won't spend more time on this. Christophe
participants (2)
-
Christophe Fergeau -
Eric Blake