Re: [Qemu-devel] [v19 24/25] cleanup QEMUOptionParameter

[Kevin Wolf <kwolf@redhat.com>]

Am 20.01.2014 um 15:20 hat Chunyan Liu geschrieben:
> Now all places using QEMUOptionParameter could use QemuOpts too, remove
> QEMUOptionParameter related code.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  block.c                   |  147 ++---------------------
>  block/cow.c               |    2 +-
>  block/qcow.c              |    2 +-
>  block/qcow2.c             |    2 +-
>  block/qed.c               |    2 +-
>  block/raw_bsd.c           |    2 +-
>  block/vhdx.c              |    2 +-
>  block/vmdk.c              |    4 +-
>  block/vvfat.c             |    2 +-
>  include/block/block.h     |    4 +-
>  include/block/block_int.h |    3 -
>  include/qemu/option.h     |   35 ------
>  qemu-img.c                |   93 ++-------------
>  util/qemu-option.c        |  294 ---------------------------------------------
>  14 files changed, 30 insertions(+), 564 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 8c490c6..b33d095 100644
> --- a/block.c
> +++ b/block.c
> @@ -394,7 +394,6 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
>  typedef struct CreateCo {
>      BlockDriver *drv;
>      char *filename;
> -    QEMUOptionParameter *options;
>      QemuOpts *opts;
>      int ret;
>      Error *err;
> @@ -403,15 +402,13 @@ typedef struct CreateCo {
>  static void coroutine_fn bdrv_create_co_entry(void *opaque)
>  {
>      Error *local_err = NULL;
> -    int ret;
> +    int ret = -1;
>  
>      CreateCo *cco = opaque;
>      assert(cco->drv);
>  
>      if (cco->drv->bdrv_create2)
>          ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err);
> -    else
> -        ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err);

The if condition isn't needed any more, it is always true.

>      if (error_is_set(&local_err)) {
>          error_propagate(&cco->err, local_err);
>      }
> @@ -324,22 +315,19 @@ fail:
>      return NULL;
>  }
>  
> -static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
> +static int add_old_style_options(const char *fmt,
>                                   QemuOpts *opts,
>                                   const char *base_filename,
>                                   const char *base_fmt)
>  {
>      if (base_filename) {
> -        if ((opts && qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) ||
> -            (list && set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename))) {
> -            error_report("Backing file not supported for file format '%s'",
> +        if (opts && qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) {            error_report("Backing file not supported for file format '%s'",

I think you lost a line break here. :-)

>                           fmt);
>              return -1;
>          }
>      }
>      if (base_fmt) {
> -        if ((opts && qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) ||
> -            (list && set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt))) {
> +        if (opts && qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>              error_report("Backing file format not supported for file "
>                           "format '%s'", fmt);
>              return -1;
> @@ -1152,7 +1140,6 @@ static int img_convert(int argc, char **argv)
>      size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>      const uint8_t *buf1;
>      BlockDriverInfo bdi;
> -    QEMUOptionParameter *param = NULL, *create_options = NULL;
>      QemuOpts *opts = NULL;
>      QemuOptsList *create_opts = NULL;
>      char *options = NULL;
> @@ -1337,7 +1324,7 @@ static int img_convert(int argc, char **argv)
>          }
>  
>          qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_sectors * 512);
> -        ret = add_old_style_options(out_fmt, NULL, opts, out_baseimg, NULL);
> +        ret = add_old_style_options(out_fmt, opts, out_baseimg, NULL);
>          if (ret < 0) {
>              goto out;
>          }
> @@ -1379,70 +1366,12 @@ static int img_convert(int argc, char **argv)
>          }
>  
>      } else {
> -        QEMUOptionParameter *out_baseimg_param;
> -
> -        create_options = append_option_parameters(create_options,
> -                                                  drv->create_options);
> -        create_options = append_option_parameters(create_options,
> -                                                  proto_drv->create_options);
> -
> -        if (options) {
> -            param = parse_option_parameters(options, create_options, param);
> -            if (param == NULL) {
> -                error_report("Invalid options for file format '%s'.", out_fmt);
> -                ret = -1;
> -                goto out;
> -            }
> -        } else {
> -            param = parse_option_parameters("", create_options, param);
> -        }
> -
> -        set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
> -        ret = add_old_style_options(out_fmt, param, NULL, out_baseimg, NULL);
> -        if (ret < 0) {
> -            goto out;
> -        }
> -
> -        /* Get backing file name if -o backing_file was used */
> -        out_baseimg_param = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> -        if (out_baseimg_param) {
> -            out_baseimg = out_baseimg_param->value.s;
> -        }
> -
> -        /* 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);
> -
> -            if (!drv->bdrv_write_compressed) {
> -                error_report("Compression not supported for this file format");
> -                ret = -1;
> -                goto out;
> -            }
> -
> -            if (encryption && encryption->value.n) {
> -                error_report("Compression and encryption not supported at "
> -                             "the same time");
> -                ret = -1;
> -                goto out;
> -            }
> -
> -            if (preallocation && preallocation->value.s
> -                && strcmp(preallocation->value.s, "off"))
> -            {
> -                error_report("Compression and preallocation not supported at "
> -                             "the same time");
> -                ret = -1;
> -                goto out;
> -            }
> -        }
> +        error_report("Not supported");
>      }

Do you really need the if condition any more? I think bdrv_create()
still returns an error like it used to, and we didn't have the check
before this series.

Kevin