[libvirt] [PATCH] test: storage: Fill in default vol types for every pool
Fill in a default volume type for every pool type, as reported by the VolGetInfo API. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- A more complete fix would take into account VolDef->type, so test driver users could set this value via volume XML. That would require adding a PostParse callback to the storage driver infrastructure. This is still an improvement in the interim and would be part of a complete fix anyways src/test/test_driver.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ce0df1f8e3..3f37b2ede7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5164,13 +5164,26 @@ testStorageVolDelete(virStorageVolPtr vol, static int testStorageVolumeTypeForPool(int pooltype) { - switch (pooltype) { - case VIR_STORAGE_POOL_DIR: - case VIR_STORAGE_POOL_FS: - case VIR_STORAGE_POOL_NETFS: - return VIR_STORAGE_VOL_FILE; - default: - return VIR_STORAGE_VOL_BLOCK; + switch ((virStoragePoolType) pooltype) { + case VIR_STORAGE_POOL_DIR: + case VIR_STORAGE_POOL_FS: + case VIR_STORAGE_POOL_NETFS: + case VIR_STORAGE_POOL_VSTORAGE: + return VIR_STORAGE_VOL_FILE; + case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_ISCSI_DIRECT: + case VIR_STORAGE_POOL_GLUSTER: + case VIR_STORAGE_POOL_RBD: + return VIR_STORAGE_VOL_NETWORK; + case VIR_STORAGE_POOL_LOGICAL: + case VIR_STORAGE_POOL_DISK: + case VIR_STORAGE_POOL_MPATH: + case VIR_STORAGE_POOL_ISCSI: + case VIR_STORAGE_POOL_SCSI: + case VIR_STORAGE_POOL_ZFS: + case VIR_STORAGE_POOL_LAST: + default: + return VIR_STORAGE_VOL_BLOCK; } } -- 2.20.1
On Thu, 2019-03-07 at 14:35 -0500, Cole Robinson wrote:
@@ -5164,13 +5164,26 @@ testStorageVolDelete(virStorageVolPtr vol, static int testStorageVolumeTypeForPool(int pooltype) { - switch (pooltype) { - case VIR_STORAGE_POOL_DIR: - case VIR_STORAGE_POOL_FS: - case VIR_STORAGE_POOL_NETFS: - return VIR_STORAGE_VOL_FILE; - default: - return VIR_STORAGE_VOL_BLOCK; + switch ((virStoragePoolType) pooltype) { + case VIR_STORAGE_POOL_DIR: + case VIR_STORAGE_POOL_FS: + case VIR_STORAGE_POOL_NETFS: + case VIR_STORAGE_POOL_VSTORAGE: + return VIR_STORAGE_VOL_FILE; + case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_ISCSI_DIRECT: + case VIR_STORAGE_POOL_GLUSTER: + case VIR_STORAGE_POOL_RBD: + return VIR_STORAGE_VOL_NETWORK; + case VIR_STORAGE_POOL_LOGICAL: + case VIR_STORAGE_POOL_DISK: + case VIR_STORAGE_POOL_MPATH: + case VIR_STORAGE_POOL_ISCSI: + case VIR_STORAGE_POOL_SCSI: + case VIR_STORAGE_POOL_ZFS:
Surely VIR_STORAGE_VOL_BLOCK should be returned here...
+ case VIR_STORAGE_POOL_LAST: + default: + return VIR_STORAGE_VOL_BLOCK; }
... and these last two would result in virReportEnumRangeError() being called? I won't comment on the actual change since it's out of my area of expertise, though it looks sane enough from where I'm standing. -- Andrea Bolognani / Red Hat / Virtualization
On 3/7/19 2:35 PM, Cole Robinson wrote:
Fill in a default volume type for every pool type, as reported by the VolGetInfo API.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- A more complete fix would take into account VolDef->type, so test driver users could set this value via volume XML. That would require adding a PostParse callback to the storage driver infrastructure. This is still an improvement in the interim and would be part of a complete fix anyways
src/test/test_driver.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)
At first I wondered if this had to do with changes I made starting at commit 4ad00278f going thru commit 538b83ae, but it doesn't seem so.
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ce0df1f8e3..3f37b2ede7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5164,13 +5164,26 @@ testStorageVolDelete(virStorageVolPtr vol, static int testStorageVolumeTypeForPool(int pooltype) { - switch (pooltype) { - case VIR_STORAGE_POOL_DIR: - case VIR_STORAGE_POOL_FS: - case VIR_STORAGE_POOL_NETFS: - return VIR_STORAGE_VOL_FILE; - default: - return VIR_STORAGE_VOL_BLOCK; + switch ((virStoragePoolType) pooltype) { + case VIR_STORAGE_POOL_DIR: + case VIR_STORAGE_POOL_FS: + case VIR_STORAGE_POOL_NETFS: + case VIR_STORAGE_POOL_VSTORAGE: + return VIR_STORAGE_VOL_FILE; + case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_ISCSI_DIRECT:
^^ This I would think should be BLOCK even though I see virISCSIDirectRefreshVol uses NETWORK at volume creation. Perhaps Michal can answer provide some insights.
+ case VIR_STORAGE_POOL_GLUSTER: + case VIR_STORAGE_POOL_RBD: + return VIR_STORAGE_VOL_NETWORK; + case VIR_STORAGE_POOL_LOGICAL: + case VIR_STORAGE_POOL_DISK: + case VIR_STORAGE_POOL_MPATH: + case VIR_STORAGE_POOL_ISCSI:
^^^ To a degree ISCSI could be NETWORK, but may also be BLOCK. Still the default seems to be BLOCK (see below)
+ case VIR_STORAGE_POOL_SCSI: + case VIR_STORAGE_POOL_ZFS: + case VIR_STORAGE_POOL_LAST:
^^^ I agree w/ Andrea that this could an Enum error; however, it doesn't seem the caller at this point can handle that.
+ default: + return VIR_STORAGE_VOL_BLOCK; } }
Using cscope I did an egrep search on "vol.*type.* = " as well as "VIR_ALLOC.*vol" in order to help find places where a volume's type may or should be set to see what kind of defaults are in use. Beyond that consider that volume's in a pool volume list are recreated at every refresh, so following the *Refresh logic to find types works pretty well too. For virStorageBackendRefreshLocal it's VIR_STORAGE_VOL_FILE For virStorageBackendSCSINewLun it's VIR_STORAGE_VOL_BLOCK (used by scsi and iscsi backends via calls to virStorageBackendSCSIFindLUs). Perhaps a "more correct" answer comes from virStoragePoolTypeInfoLookup or virStoragePoolOptionsForPoolType and adjustment of poolTypeInfo in storage_conf.c? John
On 3/8/19 7:40 AM, John Ferlan wrote:
On 3/7/19 2:35 PM, Cole Robinson wrote:
Fill in a default volume type for every pool type, as reported by the VolGetInfo API.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- A more complete fix would take into account VolDef->type, so test driver users could set this value via volume XML. That would require adding a PostParse callback to the storage driver infrastructure. This is still an improvement in the interim and would be part of a complete fix anyways
src/test/test_driver.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)
At first I wondered if this had to do with changes I made starting at commit 4ad00278f going thru commit 538b83ae, but it doesn't seem so.
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ce0df1f8e3..3f37b2ede7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5164,13 +5164,26 @@ testStorageVolDelete(virStorageVolPtr vol, static int testStorageVolumeTypeForPool(int pooltype) { - switch (pooltype) { - case VIR_STORAGE_POOL_DIR: - case VIR_STORAGE_POOL_FS: - case VIR_STORAGE_POOL_NETFS: - return VIR_STORAGE_VOL_FILE; - default: - return VIR_STORAGE_VOL_BLOCK; + switch ((virStoragePoolType) pooltype) { + case VIR_STORAGE_POOL_DIR: + case VIR_STORAGE_POOL_FS: + case VIR_STORAGE_POOL_NETFS: + case VIR_STORAGE_POOL_VSTORAGE: + return VIR_STORAGE_VOL_FILE; + case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_ISCSI_DIRECT:
^^ This I would think should be BLOCK even though I see virISCSIDirectRefreshVol uses NETWORK at volume creation.
Perhaps Michal can answer provide some insights.
My understanding of volume type is that it's more an indication of how the VM will access the resource. It has a direct effect on how tools like virt-manager will craft <disk> XML to make use of the volume. So, while a pool type=netfs NFS volume is from a network source, qemu will access it like a regular host file. It would have type=file not type=network iscsi direct advertises remote network volumes. qemu needs to access the shares over the network using its native iscsi support. so type=network
+ case VIR_STORAGE_POOL_GLUSTER: + case VIR_STORAGE_POOL_RBD: + return VIR_STORAGE_VOL_NETWORK; + case VIR_STORAGE_POOL_LOGICAL: + case VIR_STORAGE_POOL_DISK: + case VIR_STORAGE_POOL_MPATH: + case VIR_STORAGE_POOL_ISCSI:
^^^ To a degree ISCSI could be NETWORK, but may also be BLOCK. Still the default seems to be BLOCK (see below)
where as plain type='iscsi' uses iscsiadm to turn those network shares into local block devices, which qemu accesses like any other local block device. so type=block
+ case VIR_STORAGE_POOL_SCSI: + case VIR_STORAGE_POOL_ZFS: + case VIR_STORAGE_POOL_LAST:
^^^ I agree w/ Andrea that this could an Enum error; however, it doesn't seem the caller at this point can handle that.
+ default: + return VIR_STORAGE_VOL_BLOCK; } }
Using cscope I did an egrep search on "vol.*type.* = " as well as "VIR_ALLOC.*vol" in order to help find places where a volume's type may or should be set to see what kind of defaults are in use.
I basically did the same in prep for this patch. There aren't too many hits with: git grep -E "VIR_STORAGE_VOL_(FILE|BLOCK|NETWORK|NETDIR|PLOOP)"
Beyond that consider that volume's in a pool volume list are recreated at every refresh, so following the *Refresh logic to find types works pretty well too.
For virStorageBackendRefreshLocal it's VIR_STORAGE_VOL_FILE For virStorageBackendSCSINewLun it's VIR_STORAGE_VOL_BLOCK (used by scsi and iscsi backends via calls to virStorageBackendSCSIFindLUs).
Perhaps a "more correct" answer comes from virStoragePoolTypeInfoLookup or virStoragePoolOptionsForPoolType and adjustment of poolTypeInfo in storage_conf.c?
Maybe, but it's adding something to common code to fix the fairly artificial test driver case, so I'll need to validate it doesn't have any side effects to the real storage drivers. I'll post this as a v2 with the enum error bit changed. If that doesn't get an ACK i'll try and do a more complete fix at a later time, including the changes I mentioned in the commit message Thanks, Cole
participants (3)
-
Andrea Bolognani -
Cole Robinson -
John Ferlan