Re: [Qemu-devel] [PATCH V4 09/10] Use QemuOpts support in block layer

[Stefan Hajnoczi]

On Thu, Oct 25, 2012 at 08:57:27PM +0800, Dong Xu Wang wrote:
> @@ -177,9 +177,7 @@ struct BlockDriver {
>          unsigned long int req, void *buf,
>          BlockDriverCompletionFunc *cb, void *opaque);
>  
> -    /* List of options for creating images, terminated by name == NULL */
> -    QEMUOptionParameter *create_options;
> -
> +    QemuOptsList *(*bdrv_create_options)(void);

Why is .bdrv_create_options() a function?  It would be less code for
BlockDrivers to have a pointer: QemuOptsList *.

>      /* Check if compression is supported */
>      if (compress) {
> -        QEMUOptionParameter *encryption =
> -            get_option_parameter(param, BLOCK_OPT_ENCRYPT);
> -        QEMUOptionParameter *preallocation =
> -            get_option_parameter(param, BLOCK_OPT_PREALLOC);
> +        const char *encryption =
> +            qemu_opt_get(param, BLOCK_OPT_ENCRYPT);
> +        const char *preallocation =
> +            qemu_opt_get(param, BLOCK_OPT_PREALLOC);
>  
>          if (!drv->bdrv_write_compressed) {
>              error_report("Compression not supported for this file format");
> @@ -847,15 +842,15 @@ static int img_convert(int argc, char **argv)
>              goto out;
>          }
>  
> -        if (encryption && encryption->value.n) {
> +        if (encryption) {

encryption=off will print an error here.  We need to use
qemu_opt_get_bool().


> diff --git a/qemu-option.c b/qemu-option.c
> index ab270f0..c391f7d 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -553,10 +553,15 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char 
> *name, bool defval)
>  
>  uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t 
> defval)
>  {
> -    QemuOpt *opt = qemu_opt_find(opts, name);
> +    QemuOpt *opt;
> +    if (!opts) {
> +        return defval;
> +    }
> +    opt = qemu_opt_find(opts, name);
>  
> -    if (opt == NULL)
> +    if (opt == NULL) {
>          return defval;
> +    }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER);
>      return opt->value.uint;
>  }

This should be a separate patch, but there is a problem with the change
anyway: all qemu_opt_*() functions assume QemuOpts *opts is non-NULL.
This change makes qemu_opt_get_number() inconsistent.  This will lead to
bugs when people assume qemu_opt_get_bool() also handles opts == NULL,
for example.

Can the qemu-img/block callers make sure opts is never NULL or will that
add a lot of extra checks?

Stefan