[libvirt] [PATCH] test: storage: Fill in default vol types for every pool

[John Ferlan]

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 at 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