Re: [PATCH v3 4/6] qemu: update the validation for transient disk option

[Peter Krempa]

On Thu, Sep 17, 2020 at 09:30:43 -0400, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> Update validation of transient disk option. The option for qemu is supported
> with under condistions.
> 
> - qemu has blockdev feature 
> - the type is file and the format is qcow2 and raw
> - writable disk
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> ---
>  src/qemu/qemu_validate.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 070f1c962b..9cf78ca0c9 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -2002,6 +2002,28 @@ qemuValidateDomainDeviceDefDiskSerial(const char 
> *value)
>  }
>  
>  
> +static int

This is declared as returning 'int'

> +qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk,

If this function is called qemuValidateDomainDeviceDefDiskTransient it
should also do all of the validation including reporting errors.

> +                                         virQEMUCapsPtr qemuCaps)
> +{
> +    if ((!qemuCaps) || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV))

Parentheses are not required around negation

> +        return false;

but returns booleans instead

> +
> +    if (disk->src->readonly)
> +        return false;
> +
> +    if ((disk->src->format != VIR_STORAGE_FILE_QCOW2) &&
> +        (disk->src->format != VIR_STORAGE_FILE_RAW) &&
> +        (disk->src->type != VIR_STORAGE_TYPE_FILE)) {
> +        return false;
> +    }
> +
> +    if (virStorageSourceIsEmpty(disk->src))
> +        return false;
> +
> +    return true;
> +}
> +
>  static int
>  qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
>                                          virQEMUCapsPtr qemuCaps)
> @@ -2186,7 +2208,8 @@ qemuValidateDomainDeviceDefDiskFrontend(const 
> virDomainDiskDef *disk,
>          }
>      }
>  
> -    if (disk->transient) {
> +    if ((disk->transient) &&
> +        !qemuValidateDomainDeviceDefDiskTransient(disk, qemuCaps)) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("transient disks not supported yet"));

The error messages this produces are really suboptimal. You don't know
whether your qemu is old, whether incorrect format is used or whether
it's a network disk.

>          return -1;

I'll rewrite this patch to produce better error messages and fix the
problems above.