@@ -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; }

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(a)redhat.com&gt; > --- > 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?