Re: [Qemu-devel] [PATCH V10 3/4] Use QemuOpts support in block layer

[Kevin Wolf]

Am 07.01.2013 06:26, schrieb Dong Xu Wang:
> This patch will use QemuOpts related functions in block layer, add
> a member bdrv_create_options to BlockDriver struct, it will return
> a QemuOptsList pointer, which includes the image format's create
> options.
> 
> And create options's primary consumer is block creating related functions,
> so modify them together.
> 
> Signed-off-by: Dong Xu Wang <address@hidden>

> diff --git a/block/qcow.c b/block/qcow.c
> index 4276610..46aad7f 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -651,7 +651,7 @@ static void qcow_close(BlockDriverState *bs)
>      error_free(s->migration_blocker);
>  }
>  
> -static int qcow_create(const char *filename, QEMUOptionParameter *options)
> +static int qcow_create(const char *filename, QemuOpts *opts)
>  {
>      int header_size, backing_filename_len, l1_size, shift, i;
>      QCowHeader header;
> @@ -662,19 +662,16 @@ static int qcow_create(const char *filename, 
> QEMUOptionParameter *options)
>      int ret;
>      BlockDriverState *qcow_bs;
>  
> -    /* Read out options */
> -    while (options && options->name) {
> -        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            total_size = options->value.n / 512;
> -        } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> -            backing_file = options->value.s;
> -        } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
> -            flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
> +    /* Read out opts */
> +    if (opts) {

Can opts ever be NULL? (Same question for all other block drivers)

> +        total_size = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / 512;
> +        backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
> +        if (qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, 0)) {
> +            flags |= BLOCK_FLAG_ENCRYPT;
>          }
> -        options++;
>      }
>  
> -    ret = bdrv_create_file(filename, options);
> +    ret = bdrv_create_file(filename, NULL);

Why is this change correct?

Previously you could pass options to the protocol that are not supported
by the file format. For example, you can specify a backing file for raw
over sheepdog. Interestingly you keep this correct behaviour for raw,
but you seem to break it for other image formats.

>      if (ret < 0) {
>          return ret;
>      }

Kevin