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

[Stefan Hajnoczi]

On Thu, Nov 01, 2012 at 05:12:29PM +0800, Dong Xu Wang wrote:
> diff --git a/block/qcow.c b/block/qcow.c
> index b239c82..9bb736a 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,14 @@ 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;
> -        }
> -        options++;
> +    /* Read out opts */
> +    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;
>      }
>  
> -    ret = bdrv_create_file(filename, options);
> +    ret = bdrv_create_file(filename, opts);

Previously options would be an empty QEMUOptionParameter (because the
while loop iterated until options->name).  Now you are passing the same
opts for creating this image file, this seems wrong.  Should this be
bdrv_create_file(filename, NULL)?

> diff --git a/block/qcow2.c b/block/qcow2.c
> index c1ff31f..9cea051 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1180,7 +1180,7 @@ static int preallocate(BlockDriverState *bs)
>  static int qcow2_create2(const char *filename, int64_t total_size,
>                           const char *backing_file, const char 
> *backing_format,
>                           int flags, size_t cluster_size, int prealloc,
> -                         QEMUOptionParameter *options, int version)
> +                         QemuOpts *opts, int version)

The opts argument can be dropped since it is not used.

> @@ -1314,7 +1314,7 @@ out:
>      return ret;
>  }
>  
> -static int qcow2_create(const char *filename, QEMUOptionParameter *options)
> +static int qcow2_create(const char *filename, QemuOpts *opts)
>  {
>      const char *backing_file = NULL;
>      const char *backing_fmt = NULL;
> @@ -1323,45 +1323,41 @@ static int qcow2_create(const char *filename, 
> QEMUOptionParameter *options)
>      size_t cluster_size = DEFAULT_CLUSTER_SIZE;
>      int prealloc = 0;
>      int version = 2;
> +    const char *buf;
>  
>      /* Read out options */
> -    while (options && options->name) {
> -        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            sectors = 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_BACKING_FMT)) {
> -            backing_fmt = options->value.s;
> -        } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
> -            flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
> -        } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> -            if (options->value.n) {
> -                cluster_size = options->value.n;
> -            }
> -        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
> -            if (!options->value.s || !strcmp(options->value.s, "off")) {
> -                prealloc = 0;
> -            } else if (!strcmp(options->value.s, "metadata")) {
> -                prealloc = 1;
> -            } else {
> -                fprintf(stderr, "Invalid preallocation mode: '%s'\n",
> -                    options->value.s);
> -                return -EINVAL;
> -            }
> -        } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
> -            if (!options->value.s || !strcmp(options->value.s, "0.10")) {
> -                version = 2;
> -            } else if (!strcmp(options->value.s, "1.1")) {
> -                version = 3;
> -            } else {
> -                fprintf(stderr, "Invalid compatibility level: '%s'\n",
> -                    options->value.s);
> -                return -EINVAL;
> -            }
> -        } else if (!strcmp(options->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
> -            flags |= options->value.n ? BLOCK_FLAG_LAZY_REFCOUNTS : 0;
> -        }
> -        options++;

The old code worked when options == NULL.  I think the new code should
also work when opts == NULL:

if (opts) {
    ....
}

This should be done for all block drivers or the way that
bdrv_create(..., NULL) works should be changed so that block drivers
don't have to check for opts == NULL.

> @@ -674,19 +687,14 @@ static int64_t 
> raw_get_allocated_file_size(BlockDriverState *bs)
>      return (int64_t)st.st_blocks * 512;
>  }
>  
> -static int raw_create(const char *filename, QEMUOptionParameter *options)
> +static int raw_create(const char *filename, QemuOpts *opts)
>  {
>      int fd;
>      int result = 0;
>      int64_t total_size = 0;
>  
> -    /* Read out options */
> -    while (options && options->name) {
> -        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            total_size = options->value.n / BDRV_SECTOR_SIZE;
> -        }
> -        options++;
> -    }
> +    total_size = opts ?
> +        qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE : 0;

total_size is initialized to 0 above, so this line can be made more
readable like this:

if (opts) {
    total_size = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0);
}

> @@ -1081,20 +1079,15 @@ static int fd_open(BlockDriverState *bs)
>  
>  #endif /* !linux && !FreeBSD */
>  
> -static int hdev_create(const char *filename, QEMUOptionParameter *options)
> +static int hdev_create(const char *filename, QemuOpts *opts)
>  {
>      int fd;
>      int ret = 0;
>      struct stat stat_buf;
>      int64_t total_size = 0;
>  
> -    /* Read out options */
> -    while (options && options->name) {
> -        if (!strcmp(options->name, "size")) {
> -            total_size = options->value.n / BDRV_SECTOR_SIZE;
> -        }
> -        options++;
> -    }
> +    total_size = opts ?
> +        qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE : 0;

Same here.