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

[Stefan Hajnoczi]

On Fri, Nov 23, 2012 at 8:47 AM, Dong Xu Wang
<wdon...@linux.vnet.ibm.com> wrote:
> 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.
>
> This patch also define a macro called STRINGIZER, it is used to convert
> number to string.

Please use osdep.h:stringify() instead of redefining this macro.

> @@ -638,24 +638,18 @@ static int vdi_create(const char *filename, 
> QEMUOptionParameter *options)
>      logout("\n");
>
>      /* Read out options. */
> -    while (options && options->name) {
> -        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            bytes = options->value.n;
> +    if (opts) {
> +        bytes = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0);
>  #if defined(CONFIG_VDI_BLOCK_SIZE)
> -        } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> -            if (options->value.n) {
> -                /* TODO: Additional checks (SECTOR_SIZE * 2^n, ...). */
> -                block_size = options->value.n;
> -            }
> +        block_size = qemu_opt_get_size(opts,
> +                                       BLOCK_OPT_CLUSTER_SIZE,
> +                                       DEFAULT_CLUSTER_SIZE);

Please preserve the TODO comment.

>  #endif
>  #if defined(CONFIG_VDI_STATIC_IMAGE)
> -        } else if (!strcmp(options->name, BLOCK_OPT_STATIC)) {
> -            if (options->value.n) {
> -                image_type = VDI_TYPE_STATIC;
> -            }
> -#endif
> +        if (qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, 0)) {
> +            image_type = VDI_TYPE_STATIC;

s/BLOCK_OPT_ENCRYPT/BLOCK_OPT_STATIC/

>      int disk_type;
>      int ret = -EIO;
>
> -    /* Read out options */
> -    total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
> -
> -    disk_type_param = get_option_parameter(options, BLOCK_OPT_SUBFMT);
> -    if (disk_type_param && disk_type_param->value.s) {
> -        if (!strcmp(disk_type_param->value.s, "dynamic")) {
> -            disk_type = VHD_DYNAMIC;
> -        } else if (!strcmp(disk_type_param->value.s, "fixed")) {
> -            disk_type = VHD_FIXED;
> +    /* Read out opts */
> +    if (opts) {
> +        total_size = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0);
> +        disk_type_param = qemu_opt_get(opts, BLOCK_OPT_SUBFMT);
> +        if (disk_type_param) {
> +            if (!strcmp(disk_type_param, "dynamic")) {
> +                disk_type = VHD_DYNAMIC;
> +            } else if (!strcmp(disk_type_param, "fixed")) {
> +                disk_type = VHD_FIXED;
> +            } else {
> +                return -EINVAL;
> +            }
>          } else {
> -            return -EINVAL;
> +            disk_type = VHD_DYNAMIC;
>          }
> -    } else {
> -        disk_type = VHD_DYNAMIC;
>      }

disk_type must be initialized even when opts == NULL.  It's easiest to do:
int disk_type = VHD_DYNAMIC;
...
if (opts) {
    ...
    if (disk_type_param) {
        if (!strcmp(disk_type_param, "dynamic")) {
            disk_type = VHD_DYNAMIC;
        } else if (!strcmp(disk_type_param, "fixed")) {
            disk_type = VHD_FIXED;
        } else {
            return -EINVAL;
        }
    }
}