Re: [Qemu-devel] [PATCH V26 13/32] vvfat.c: handle cross_driver's create_options and create_opts

Re: [Qemu-devel] [PATCH V26 13/32] vvfat.c: handle cross_driver's create_options and create_opts

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

On 04/29/2014 03:10 AM, Chunyan Liu wrote:
> vvfat shares create options of qcow driver. To avoid vvfat broken when

s/broken/breaking/

> qcow driver changes from QEMUOptionParameter to QemuOpts, let it able
> to handle both cases.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  block/vvfat.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 155fc9b..b49ec96 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2907,7 +2907,8 @@ static BlockDriver vvfat_write_target = {
>  static int enable_write_target(BDRVVVFATState *s)
>  {
>      BlockDriver *bdrv_qcow;
> -    QEMUOptionParameter *options;
> +    QemuOptsList *create_opts = NULL;
> +    QemuOpts *opts = NULL;
>      Error *local_err = NULL;
>      int ret;
>      int size = sector2cluster(s, s->sector_count);
> @@ -2922,11 +2923,17 @@ static int enable_write_target(BDRVVVFATState *s)
>      }
>  
>      bdrv_qcow = bdrv_find_format("qcow");
> -    options = parse_option_parameters("", bdrv_qcow->create_options, NULL);
> -    set_option_parameter_int(options, BLOCK_OPT_SIZE, s->sector_count * 512);
> -    set_option_parameter(options, BLOCK_OPT_BACKING_FILE, "fat:");
> +    assert(!(bdrv_qcow->create_opts && bdrv_qcow->create_options));
> +    if (bdrv_qcow->create_options) {
> +        create_opts = params_to_opts(bdrv_qcow->create_options);

allocated...

> +    } else {
> +        create_opts = bdrv_qcow->create_opts;

vs. reference...

>  err:
> +    qemu_opts_del(opts);
> +    qemu_opts_free(create_opts);

but unconditionally freed on either path.  Calling
g_free(bdrv_qcow->create_opts) is not nice.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V26 13/32] vvfat.c: handle cross_driver's create_options and create_opts

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

On 05/01/2014 01:18 PM, Eric Blake wrote:
> On 04/29/2014 03:10 AM, Chunyan Liu wrote:
>> vvfat shares create options of qcow driver. To avoid vvfat broken when
> 
> s/broken/breaking/
> 
>> qcow driver changes from QEMUOptionParameter to QemuOpts, let it able
>> to handle both cases.
>>

>> +    if (bdrv_qcow->create_options) {
>> +        create_opts = params_to_opts(bdrv_qcow->create_options);
> 
> allocated...
> 
>> +    } else {
>> +        create_opts = bdrv_qcow->create_opts;
> 
> vs. reference...
> 

Couldn't you just do:

create_opts = qemu_opts_append(NULL, bdrv_qcow2->create_opts,
                               bdrv_qcow2->create_options);

at which point, your blind free on cleanup will just work?

>>  err:
>> +    qemu_opts_del(opts);
>> +    qemu_opts_free(create_opts);
> 
> but unconditionally freed on either path.  Calling
> g_free(bdrv_qcow->create_opts) is not nice.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V26 19/32] qcow2.c: replace QEMUOptionParameter with QemuOpts

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2970 bytes --]

On 04/29/2014 03:10 AM, Chunyan Liu wrote:
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  block/qcow2.c         | 265 +++++++++++++++++++++++++++-----------------------
>  include/qemu/option.h |   1 +
>  util/qemu-option.c    |   2 +-
>  3 files changed, 143 insertions(+), 125 deletions(-)
> 
> @@ -2375,10 +2392,10 @@ static BlockDriver bdrv_qcow2 = {
>      .bdrv_open          = qcow2_open,
>      .bdrv_close         = qcow2_close,
>      .bdrv_reopen_prepare  = qcow2_reopen_prepare,
> -    .bdrv_create        = qcow2_create,
> -    .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_create2         = qcow2_create,
> +    .bdrv_has_zero_init   = bdrv_has_zero_init_1,
>      .bdrv_co_get_block_status = qcow2_co_get_block_status,
> -    .bdrv_set_key       = qcow2_set_key,
> +    .bdrv_set_key           = qcow2_set_key,

Looks odd to be re-indenting some, but not all, of the existing
elements, particularly when you aren't reindenting them to the same
depth.  But as the indentation is already a mess here, you aren't making
it worse, so it doesn't impact my review.

> +++ b/include/qemu/option.h
> @@ -130,6 +130,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name);
>   * Returns: true if @opts includes 'help' or equivalent.
>   */
>  bool qemu_opt_has_help_opt(QemuOpts *opts);
> +QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name);
>  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);
>  uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 2667e16..cb92b42 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -568,7 +568,7 @@ void qemu_opts_print_help(QemuOptsList *list)
>  }
>  /* ------------------------------------------------------------------ */
>  
> -static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
> +QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>  {

I would have used a separate commit for exporting qemu_opt_find as a
non-static function.  But I can live with it here, if it will get this
in the tree sooner.

Reviewed-by: Eric Blake <eblake@redhat.com>

But if you DO respin this series, then when you split the function
export into its own patch, please also consider adding documentation to
the function (undocumented static functions are generally okay, because
you already have the file open and can just read the function; but
undocumented exported functions risk a caller expecting one semantic,
but the implementation providing another, and the two getting out of
sync because there was nothing written to document what was expected).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V26 12/32] change block layer to support both QemuOpts and QEMUOptionParamter

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:10:36PM +0800, Chunyan Liu wrote:
> Change block layer to support both QemuOpts and QEMUOptionParameter.
> After this patch, it will change backend drivers one by one. At the end,
> QEMUOptionParameter will be removed and only QemuOpts is kept.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>


Did you send the patch series twice? I commented in the first v26 emails...


-- 
Leandro Dorileo

> ---
> Changes to V25:
>   * fix Eric's comments:
>   * update bdrv_create_co_entry and bdrv_amend_options code, to let it
>     more readable.
>   * add assertion in bdrv_register.
>   * improve comments to create_opts in header file.
> 
>  block.c                   | 158 ++++++++++++++++++++++++++++++++--------------
>  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     |   7 +-
>  include/block/block_int.h |  13 +++-
>  qemu-img.c                |  94 +++++++++++++--------------
>  12 files changed, 180 insertions(+), 110 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4745712..124f045 100644
> --- a/block.c
> +++ b/block.c
> @@ -328,6 +328,13 @@ void bdrv_register(BlockDriver *bdrv)
>          }
>      }
>  
> +    if (bdrv->bdrv_create) {
> +        assert(!bdrv->bdrv_create2 && !bdrv->create_opts);
> +        assert(!bdrv->bdrv_amend_options2);
> +    } else if (bdrv->bdrv_create2) {
> +        assert(!bdrv->bdrv_create && !bdrv->create_options);
> +        assert(!bdrv->bdrv_amend_options);
> +    }
>      QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list);
>  }
>  
> @@ -419,6 +426,7 @@ typedef struct CreateCo {
>      BlockDriver *drv;
>      char *filename;
>      QEMUOptionParameter *options;
> +    QemuOpts *opts;
>      int ret;
>      Error *err;
>  } CreateCo;
> @@ -430,8 +438,28 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
>  
>      CreateCo *cco = opaque;
>      assert(cco->drv);
> +    assert(!(cco->options && cco->opts));
>  
> -    ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err);
> +    if (cco->drv->bdrv_create2) {
> +        QemuOptsList *opts_list = NULL;
> +        if (cco->options) {
> +            opts_list = params_to_opts(cco->options);
> +            cco->opts = qemu_opts_create(opts_list, NULL, 0, &error_abort);
> +        }
> +        ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err);
> +        if (cco->options) {
> +            qemu_opts_del(cco->opts);
> +            qemu_opts_free(opts_list);
> +        }
> +    } else {
> +        if (cco->opts) {
> +            cco->options = opts_to_params(cco->opts);
> +        }
> +        ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err);
> +        if (cco->opts) {
> +            free_option_parameters(cco->options);
> +        }
> +    }
>      if (local_err) {
>          error_propagate(&cco->err, local_err);
>      }
> @@ -439,7 +467,8 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
>  }
>  
>  int bdrv_create(BlockDriver *drv, const char* filename,
> -    QEMUOptionParameter *options, Error **errp)
> +                QEMUOptionParameter *options,
> +                QemuOpts *opts, Error **errp)
>  {
>      int ret;
>  
> @@ -448,11 +477,12 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>          .drv = drv,
>          .filename = g_strdup(filename),
>          .options = options,
> +        .opts = opts,
>          .ret = NOT_DONE,
>          .err = NULL,
>      };
>  
> -    if (!drv->bdrv_create) {
> +    if (!drv->bdrv_create && !drv->bdrv_create2) {
>          error_setg(errp, "Driver '%s' does not support image creation", drv->format_name);
>          ret = -ENOTSUP;
>          goto out;
> @@ -484,7 +514,7 @@ out:
>  }
>  
>  int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
> -                     Error **errp)
> +                     QemuOpts *opts, Error **errp)
>  {
>      BlockDriver *drv;
>      Error *local_err = NULL;
> @@ -496,7 +526,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
>          return -ENOENT;
>      }
>  
> -    ret = bdrv_create(drv, filename, options, &local_err);
> +    ret = bdrv_create(drv, filename, options, opts, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>      }
> @@ -1184,7 +1214,8 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp)
>      char *tmp_filename = g_malloc0(PATH_MAX + 1);
>      int64_t total_size;
>      BlockDriver *bdrv_qcow2;
> -    QEMUOptionParameter *create_options;
> +    QemuOptsList *create_opts = NULL;
> +    QemuOpts *opts = NULL;
>      QDict *snapshot_options;
>      BlockDriverState *bs_snapshot;
>      Error *local_err;
> @@ -1209,13 +1240,20 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp)
>      }
>  
>      bdrv_qcow2 = bdrv_find_format("qcow2");
> -    create_options = parse_option_parameters("", bdrv_qcow2->create_options,
> -                                             NULL);
> -
> -    set_option_parameter_int(create_options, BLOCK_OPT_SIZE, total_size);
>  
> -    ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, &local_err);
> -    free_option_parameters(create_options);
> +    assert(!(bdrv_qcow2->create_options && bdrv_qcow2->create_opts));
> +    if (bdrv_qcow2->create_options) {
> +        create_opts = params_to_opts(bdrv_qcow2->create_options);
> +    } else {
> +        create_opts = bdrv_qcow2->create_opts;
> +    }
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
> +    ret = bdrv_create(bdrv_qcow2, tmp_filename, NULL, opts, &local_err);
> +    qemu_opts_del(opts);
> +    if (bdrv_qcow2->create_options) {
> +        qemu_opts_free(create_opts);
> +    }
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not create temporary overlay "
>                           "'%s': %s", tmp_filename,
> @@ -5310,8 +5348,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                       char *options, uint64_t img_size, int flags,
>                       Error **errp, bool quiet)
>  {
> -    QEMUOptionParameter *param = NULL, *create_options = NULL;
> -    QEMUOptionParameter *backing_fmt, *backing_file, *size;
> +    QemuOptsList *create_opts = NULL;
> +    QemuOpts *opts = NULL;
> +    const char *backing_fmt, *backing_file;
> +    int64_t size;
>      BlockDriver *drv, *proto_drv;
>      BlockDriver *backing_drv = NULL;
>      Error *local_err = NULL;
> @@ -5330,28 +5370,25 @@ void bdrv_img_create(const char *filename, const char *fmt,
>          return;
>      }
>  
> -    create_options = append_option_parameters(create_options,
> -                                              drv->create_options);
> -    create_options = append_option_parameters(create_options,
> -                                              proto_drv->create_options);
> +    create_opts = qemu_opts_append(create_opts, drv->create_opts,
> +                                   drv->create_options);
> +    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts,
> +                                   proto_drv->create_options);
>  
>      /* Create parameter list with default values */
> -    param = parse_option_parameters("", create_options, param);
> -
> -    set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
>  
>      /* Parse -o options */
>      if (options) {
> -        param = parse_option_parameters(options, create_options, param);
> -        if (param == NULL) {
> +        if (qemu_opts_do_parse(opts, options, NULL) != 0) {
>              error_setg(errp, "Invalid options for file format '%s'.", fmt);
>              goto out;
>          }
>      }
>  
>      if (base_filename) {
> -        if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
> -                                 base_filename)) {
> +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) {
>              error_setg(errp, "Backing file not supported for file format '%s'",
>                         fmt);
>              goto out;
> @@ -5359,37 +5396,37 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      }
>  
>      if (base_fmt) {
> -        if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>              error_setg(errp, "Backing file format not supported for file "
>                               "format '%s'", fmt);
>              goto out;
>          }
>      }
>  
> -    backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> -    if (backing_file && backing_file->value.s) {
> -        if (!strcmp(filename, backing_file->value.s)) {
> +    backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
> +    if (backing_file) {
> +        if (!strcmp(filename, backing_file)) {
>              error_setg(errp, "Error: Trying to create an image with the "
>                               "same filename as the backing file");
>              goto out;
>          }
>      }
>  
> -    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
> -    if (backing_fmt && backing_fmt->value.s) {
> -        backing_drv = bdrv_find_format(backing_fmt->value.s);
> +    backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
> +    if (backing_fmt) {
> +        backing_drv = bdrv_find_format(backing_fmt);
>          if (!backing_drv) {
>              error_setg(errp, "Unknown backing file format '%s'",
> -                       backing_fmt->value.s);
> +                       backing_fmt);
>              goto out;
>          }
>      }
>  
>      // The size for the image must always be specified, with one exception:
>      // If we are using a backing file, we can obtain the size from there
> -    size = get_option_parameter(param, BLOCK_OPT_SIZE);
> -    if (size && size->value.n == -1) {
> -        if (backing_file && backing_file->value.s) {
> +    size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> +    if (size == -1) {
> +        if (backing_file) {
>              BlockDriverState *bs;
>              uint64_t size;
>              char buf[32];
> @@ -5400,11 +5437,11 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                  flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>  
>              bs = NULL;
> -            ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags,
> +            ret = bdrv_open(&bs, backing_file, NULL, NULL, back_flags,
>                              backing_drv, &local_err);
>              if (ret < 0) {
>                  error_setg_errno(errp, -ret, "Could not open '%s': %s",
> -                                 backing_file->value.s,
> +                                 backing_file,
>                                   error_get_pretty(local_err));
>                  error_free(local_err);
>                  local_err = NULL;
> @@ -5414,7 +5451,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>              size *= 512;
>  
>              snprintf(buf, sizeof(buf), "%" PRId64, size);
> -            set_option_parameter(param, BLOCK_OPT_SIZE, buf);
> +            qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size);
>  
>              bdrv_unref(bs);
>          } else {
> @@ -5425,16 +5462,18 @@ void bdrv_img_create(const char *filename, const char *fmt,
>  
>      if (!quiet) {
>          printf("Formatting '%s', fmt=%s ", filename, fmt);
> -        print_option_parameters(param);
> +        qemu_opts_print(opts);
>          puts("");
>      }
> -    ret = bdrv_create(drv, filename, param, &local_err);
> +
> +    ret = bdrv_create(drv, filename, NULL, opts, &local_err);
> +
>      if (ret == -EFBIG) {
>          /* This is generally a better message than whatever the driver would
>           * deliver (especially because of the cluster_size_hint), since that
>           * is most probably not much different from "image too large". */
>          const char *cluster_size_hint = "";
> -        if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) {
> +        if (qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE, 0)) {
>              cluster_size_hint = " (try using a larger cluster size)";
>          }
>          error_setg(errp, "The image size is too large for file format '%s'"
> @@ -5444,9 +5483,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      }
>  
>  out:
> -    free_option_parameters(create_options);
> -    free_option_parameters(param);
> -
> +    qemu_opts_del(opts);
> +    qemu_opts_free(create_opts);
>      if (local_err) {
>          error_propagate(errp, local_err);
>      }
> @@ -5464,12 +5502,36 @@ void bdrv_add_before_write_notifier(BlockDriverState *bs,
>      notifier_with_return_list_add(&bs->before_write_notifiers, notifier);
>  }
>  
> -int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
> +int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options,
> +                       QemuOpts *opts)
>  {
> -    if (bs->drv->bdrv_amend_options == NULL) {
> +    int ret;
> +    assert(!(options && opts));
> +
> +    if (!bs->drv->bdrv_amend_options && !bs->drv->bdrv_amend_options2) {
>          return -ENOTSUP;
>      }
> -    return bs->drv->bdrv_amend_options(bs, options);
> +    if (bs->drv->bdrv_amend_options2) {
> +        QemuOptsList *opts_list = NULL;
> +        if (options) {
> +            opts_list = params_to_opts(options);
> +            opts = qemu_opts_create(opts_list, NULL, 0, &error_abort);
> +        }
> +        ret = bs->drv->bdrv_amend_options2(bs, opts);
> +        if (options) {
> +            qemu_opts_del(opts);
> +            qemu_opts_free(opts_list);
> +        }
> +    } else {
> +        if (opts) {
> +            options = opts_to_params(opts);
> +        }
> +        ret = bs->drv->bdrv_amend_options(bs, options);
> +        if (opts) {
> +            free_option_parameters(options);
> +        }
> +    }
> +    return ret;
>  }
>  
>  /* This function will be called by the bdrv_recurse_is_first_non_filter method
> diff --git a/block/cow.c b/block/cow.c
> index 30deb88..26cf4a5 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -345,7 +345,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>          options++;
>      }
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return ret;
> diff --git a/block/qcow.c b/block/qcow.c
> index d5a7d5f..9411aef 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -687,7 +687,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
>          options++;
>      }
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return ret;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index e903d97..49985e3 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1625,7 +1625,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return ret;
> diff --git a/block/qed.c b/block/qed.c
> index c130e42..2982640 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -566,7 +566,7 @@ static int qed_create(const char *filename, uint32_t cluster_size,
>      int ret = 0;
>      BlockDriverState *bs;
>  
> -    ret = bdrv_create_file(filename, NULL, &local_err);
> +    ret = bdrv_create_file(filename, NULL, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return ret;
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 01ea692..9ae5fc2 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -145,7 +145,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, NULL, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>      }
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 509baaf..a9fcf6b 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1796,7 +1796,7 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
>      block_size = block_size > VHDX_BLOCK_SIZE_MAX ? VHDX_BLOCK_SIZE_MAX :
>                                                      block_size;
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto exit;
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 06a1f9f..3802863 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1526,7 +1526,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
>      uint32_t *gd_buf = NULL;
>      int gd_buf_size;
>  
> -    ret = bdrv_create_file(filename, NULL, &local_err);
> +    ret = bdrv_create_file(filename, NULL, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto exit;
> @@ -1866,7 +1866,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>      if (!split && !flat) {
>          desc_offset = 0x200;
>      } else {
> -        ret = bdrv_create_file(filename, options, &local_err);
> +        ret = bdrv_create_file(filename, options, NULL, &local_err);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret, "Could not create image file");
>              goto exit;
> diff --git a/block/vvfat.c b/block/vvfat.c
> index c3af7ff..155fc9b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2926,7 +2926,7 @@ static int enable_write_target(BDRVVVFATState *s)
>      set_option_parameter_int(options, BLOCK_OPT_SIZE, s->sector_count * 512);
>      set_option_parameter(options, BLOCK_OPT_BACKING_FILE, "fat:");
>  
> -    ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, &local_err);
> +    ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, NULL, &local_err);
>      if (ret < 0) {
>          qerror_report_err(local_err);
>          error_free(local_err);
> diff --git a/include/block/block.h b/include/block/block.h
> index c12808a..bfa2192 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -177,9 +177,9 @@ BlockDriver *bdrv_find_format(const char *format_name);
>  BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
>                                            bool readonly);
>  int bdrv_create(BlockDriver *drv, const char* filename,
> -    QEMUOptionParameter *options, Error **errp);
> +    QEMUOptionParameter *options, QemuOpts *opts, Error **errp);
>  int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
> -                     Error **errp);
> +                     QemuOpts *opts, Error **errp);
>  BlockDriverState *bdrv_new(const char *device_name, Error **errp);
>  void bdrv_make_anon(BlockDriverState *bs);
>  void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
> @@ -284,7 +284,8 @@ typedef enum {
>  
>  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>  
> -int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
> +int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options,
> +                       QemuOpts *opts);
>  
>  /* external snapshots */
>  bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index cd5bc73..59875db 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -118,6 +118,8 @@ struct BlockDriver {
>      void (*bdrv_rebind)(BlockDriverState *bs);
>      int (*bdrv_create)(const char *filename, QEMUOptionParameter *options,
>                         Error **errp);
> +    /* FIXME: will remove the duplicate and rename back to bdrv_create later */
> +    int (*bdrv_create2)(const char *filename, QemuOpts *opts, Error **errp);
>      int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
>      int (*bdrv_make_empty)(BlockDriverState *bs);
>      /* aio */
> @@ -217,7 +219,12 @@ struct BlockDriver {
>  
>      /* List of options for creating images, terminated by name == NULL */
>      QEMUOptionParameter *create_options;
> -
> +    /* FIXME: will replace create_options.
> +     * These two fields are mutually exclusive. At most one is non-NULL.
> +     * create_options should only be set with bdrv_create, and create_opts
> +     * should only be set with bdrv_create2.
> +     */
> +    QemuOptsList *create_opts;
>  
>      /*
>       * Returns 0 for completed check, -errno for internal errors.
> @@ -228,6 +235,10 @@ struct BlockDriver {
>  
>      int (*bdrv_amend_options)(BlockDriverState *bs,
>          QEMUOptionParameter *options);
> +    /* FIXME: will remove the duplicate and rename back to
> +     * bdrv_amend_options later
> +     */
> +    int (*bdrv_amend_options2)(BlockDriverState *bs, QemuOpts *opts);
>  
>      void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event);
>  
> diff --git a/qemu-img.c b/qemu-img.c
> index 968b4c8..14d3acd 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -249,7 +249,7 @@ static int read_password(char *buf, int buf_size)
>  static int print_block_option_help(const char *filename, const char *fmt)
>  {
>      BlockDriver *drv, *proto_drv;
> -    QEMUOptionParameter *create_options = NULL;
> +    QemuOptsList *create_opts = NULL;
>  
>      /* Find driver and parse its options */
>      drv = bdrv_find_format(fmt);
> @@ -258,21 +258,20 @@ static int print_block_option_help(const char *filename, const char *fmt)
>          return 1;
>      }
>  
> -    create_options = append_option_parameters(create_options,
> -                                              drv->create_options);
> -
> +    create_opts = qemu_opts_append(create_opts, drv->create_opts,
> +                                   drv->create_options);
>      if (filename) {
>          proto_drv = bdrv_find_protocol(filename, true);
>          if (!proto_drv) {
>              error_report("Unknown protocol '%s'", filename);
>              return 1;
>          }
> -        create_options = append_option_parameters(create_options,
> -                                                  proto_drv->create_options);
> +        create_opts = qemu_opts_append(create_opts, proto_drv->create_opts,
> +                                       proto_drv->create_options);
>      }
>  
> -    print_option_help(create_options);
> -    free_option_parameters(create_options);
> +    qemu_opts_print_help(create_opts);
> +    qemu_opts_free(create_opts);
>      return 0;
>  }
>  
> @@ -326,19 +325,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 (set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename)) {
> +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) {
>              error_report("Backing file not supported for file format '%s'",
>                           fmt);
>              return -1;
>          }
>      }
>      if (base_fmt) {
> -        if (set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>              error_report("Backing file format not supported for file "
>                           "format '%s'", fmt);
>              return -1;
> @@ -1169,8 +1168,9 @@ 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;
> -    QEMUOptionParameter *out_baseimg_param;
> +    QemuOpts *opts = NULL;
> +    QemuOptsList *create_opts = NULL;
> +    const char *out_baseimg_param;
>      char *options = NULL;
>      const char *snapshot_name = NULL;
>      int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
> @@ -1359,40 +1359,36 @@ static int img_convert(int argc, char **argv)
>          goto out;
>      }
>  
> -    create_options = append_option_parameters(create_options,
> -                                              drv->create_options);
> -    create_options = append_option_parameters(create_options,
> -                                              proto_drv->create_options);
> +    create_opts = qemu_opts_append(create_opts, drv->create_opts,
> +                                   drv->create_options);
> +    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts,
> +                                   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);
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    if (options && qemu_opts_do_parse(opts, options, NULL)) {
> +        error_report("Invalid options for file format '%s'.", out_fmt);
> +        ret = -1;
> +        goto out;
>      }
>  
> -    set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
> -    ret = add_old_style_options(out_fmt, param, out_baseimg, NULL);
> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_sectors * 512);
> +    ret = add_old_style_options(out_fmt, opts, 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);
> +    out_baseimg_param = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
>      if (out_baseimg_param) {
> -        out_baseimg = out_baseimg_param->value.s;
> +        out_baseimg = out_baseimg_param;
>      }
>  
>      /* 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);
> +        bool encryption =
> +            qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, false);
> +        const char *preallocation =
> +            qemu_opt_get(opts, BLOCK_OPT_PREALLOC);
>  
>          if (!drv->bdrv_write_compressed) {
>              error_report("Compression not supported for this file format");
> @@ -1400,15 +1396,15 @@ static int img_convert(int argc, char **argv)
>              goto out;
>          }
>  
> -        if (encryption && encryption->value.n) {
> +        if (encryption) {
>              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"))
> +        if (preallocation
> +            && strcmp(preallocation, "off"))
>          {
>              error_report("Compression and preallocation not supported at "
>                           "the same time");
> @@ -1419,7 +1415,7 @@ static int img_convert(int argc, char **argv)
>  
>      if (!skip_create) {
>          /* Create the new image */
> -        ret = bdrv_create(drv, out_filename, param, &local_err);
> +        ret = bdrv_create(drv, out_filename, NULL, opts, &local_err);
>          if (ret < 0) {
>              error_report("%s: error while converting %s: %s",
>                           out_filename, out_fmt, error_get_pretty(local_err));
> @@ -1683,8 +1679,8 @@ out:
>          qemu_progress_print(100, 0);
>      }
>      qemu_progress_end();
> -    free_option_parameters(create_options);
> -    free_option_parameters(param);
> +    qemu_opts_del(opts);
> +    qemu_opts_free(create_opts);
>      qemu_vfree(buf);
>      if (sn_opts) {
>          qemu_opts_del(sn_opts);
> @@ -2675,7 +2671,8 @@ static int img_amend(int argc, char **argv)
>  {
>      int c, ret = 0;
>      char *options = NULL;
> -    QEMUOptionParameter *create_options = NULL, *options_param = NULL;
> +    QemuOptsList *create_opts = NULL;
> +    QemuOpts *opts = NULL;
>      const char *fmt = NULL, *filename;
>      bool quiet = false;
>      BlockDriverState *bs = NULL;
> @@ -2746,17 +2743,16 @@ static int img_amend(int argc, char **argv)
>          goto out;
>      }
>  
> -    create_options = append_option_parameters(create_options,
> -            bs->drv->create_options);
> -    options_param = parse_option_parameters(options, create_options,
> -            options_param);
> -    if (options_param == NULL) {
> +    create_opts = qemu_opts_append(create_opts, bs->drv->create_opts,
> +                                   bs->drv->create_options);
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    if (options && qemu_opts_do_parse(opts, options, NULL)) {
>          error_report("Invalid options for file format '%s'", fmt);
>          ret = -1;
>          goto out;
>      }
>  
> -    ret = bdrv_amend_options(bs, options_param);
> +    ret = bdrv_amend_options(bs, NULL, opts);
>      if (ret < 0) {
>          error_report("Error while amending options: %s", strerror(-ret));
>          goto out;
> @@ -2766,8 +2762,8 @@ out:
>      if (bs) {
>          bdrv_unref(bs);
>      }
> -    free_option_parameters(create_options);
> -    free_option_parameters(options_param);
> +    qemu_opts_del(opts);
> +    qemu_opts_free(create_opts);
>      g_free(options);
>  
>      if (ret) {
> -- 
> 1.8.4.5
> 
> 

Re: [Qemu-devel] [PATCH V26 12/32] change block layer to support both QemuOpts and QEMUOptionParamter

[Chun Yan Liu]

>>> On 4/30/2014 at 09:57 AM, in message <5360587E.5050409@redhat.com>, Eric Blake
<eblake@redhat.com> wrote: 
> On 04/29/2014 03:10 AM, Chunyan Liu wrote: 
> > Change block layer to support both QemuOpts and QEMUOptionParameter. 
> > After this patch, it will change backend drivers one by one. At the end, 
> > QEMUOptionParameter will be removed and only QemuOpts is kept. 
> >  
> > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> 
> > Signed-off-by: Chunyan Liu <cyliu@suse.com> 
> > --- 
> > Changes to V25: 
> >   * fix Eric's comments: 
> >   * update bdrv_create_co_entry and bdrv_amend_options code, to let it 
> >     more readable. 
> >   * add assertion in bdrv_register. 
> >   * improve comments to create_opts in header file. 
> >  
> >  block.c                   | 158  
> ++++++++++++++++++++++++++++++++-------------- 
> >  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     |   7 +- 
> >  include/block/block_int.h |  13 +++- 
> >  qemu-img.c                |  94 +++++++++++++-------------- 
> >  12 files changed, 180 insertions(+), 110 deletions(-) 
> >  
>  
> >   
> > -    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); 
> > +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort); 
> > +    if (options && qemu_opts_do_parse(opts, options, NULL)) { 
> > +        error_report("Invalid options for file format '%s'.", out_fmt); 
>  
> Pre-existing, but error messages shouldn't end in '.'  Trivial enough to 
> not stop the review. 

Didn't modify it since to avoid print info change, let qemu-iotests PASS.
With a simple grep, there are many places in current code where  error
messages are ended in '.', could change all in a time maybe.

>  
> Yay! I like it!  It's looking a lot better with all the fixes you've 
> folded in. 
>  
> Reviewed-by: Eric Blake <eblake@redhat.com> 
>  
> --  
> Eric Blake   eblake redhat com    +1-919-301-3266 
> Libvirt virtualization library http://libvirt.org 
>  
>  

Re: [Qemu-devel] [PATCH V26 20/32] qed.c: replace QEMUOptionParameter with QemuOpts

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

On 04/29/2014 03:10 AM, Chunyan Liu wrote:
> One extra change is to define QED_DEFAULT_CLUSTER_SIZE = 65536 instead
> of 64 * 1024; because:
> according to existing create_options, "cluster size" has default value =
> QED_DEFAULT_CLUSTER_SIZE, after switching to create_opts, this has to be
> stringized and set to .def_value_str. That is,
>   .def_value_str = stringify(QED_DEFAULT_CLUSTER_SIZE),
> so the QED_DEFAULT_CLUSTER_SIZE could not be a expression.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  block/qed.c | 114 ++++++++++++++++++++++++++++++++----------------------------
>  block/qed.h |   3 +-
>  2 files changed, 61 insertions(+), 56 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V26 20/32] qed.c: replace QEMUOptionParameter with QemuOpts

[Stefan Hajnoczi]

On Tue, Apr 29, 2014 at 05:10:44PM +0800, Chunyan Liu wrote:
> One extra change is to define QED_DEFAULT_CLUSTER_SIZE = 65536 instead
> of 64 * 1024; because:
> according to existing create_options, "cluster size" has default value =
> QED_DEFAULT_CLUSTER_SIZE, after switching to create_opts, this has to be
> stringized and set to .def_value_str. That is,
>   .def_value_str = stringify(QED_DEFAULT_CLUSTER_SIZE),
> so the QED_DEFAULT_CLUSTER_SIZE could not be a expression.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  block/qed.c | 114 ++++++++++++++++++++++++++++++++----------------------------
>  block/qed.h |   3 +-
>  2 files changed, 61 insertions(+), 56 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH V26 02/32] QemuOpts: add def_value_str to QemuOptDesc

[Stefan Hajnoczi]

On Tue, Apr 29, 2014 at 05:10:26PM +0800, Chunyan Liu wrote:
> Add def_value_str (default value) to QemuOptDesc, to replace function of the
> default value in QEMUOptionParameter.
> 
> Improve qemu_opts_get_* functions: if find opt, return opt->str; otherwise,
> if desc->def_value_str is set, return desc->def_value_str; otherwise, return
> input defval.
> 
> Improve qemu_opts_print: if option is set, print opt->str; otherwise, if
> desc->def_value_str is set, also print it. It will replace
> print_option_parameters.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
> Changes to V25:
>   * split v25 patch into two: this one and next one.
>   * this patch is the same as v22 which is reviewed-by Eric.
> 
>  include/qemu/option.h |  3 ++-
>  util/qemu-option.c    | 56 ++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 49 insertions(+), 10 deletions(-)

Modulo Eric's comment:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH V26 03/32] QemuOpts: repurpose qemu_opts_print to replace print_option_parameters

[Chun Yan Liu]

>>> On 4/30/2014 at 12:30 AM, in message <535FD3B6.6020701@redhat.com>, Eric Blake
<eblake@redhat.com> wrote: 
> On 04/29/2014 03:10 AM, Chunyan Liu wrote: 
> > Currently this function is not used anywhere. In later patches, it will 
> > replace print_option_parameters. To avoid print info changes, change 
> > qemu_opts_print from fprintf stderr to printf to keep consistent with 
> > print_option_parameters, remove last printf and print size/number with 
> > opt->value.uint instead of opt->str. 
> >  
> > Signed-off-by: Chunyan Liu <cyliu@suse.com> 
> > --- 
> >  util/qemu-option.c | 10 ++++++---- 
> >  1 file changed, 6 insertions(+), 4 deletions(-) 
>  
> I would prefer to see this patch first, and 2/32 second; I'd also prefer 
> to see the signature change (changing int to void return, and dropping 
> dummy parameter) in this patch. 
>  
> > +        } else if ((desc->type == QEMU_OPT_SIZE || 
> > +                    desc->type == QEMU_OPT_NUMBER) && opt) { 
> > +            printf("%s=%" PRId64 " ", desc->name, opt->value.uint); 
>  
> Shouldn't this  be PRIu64, since you are printing an unsigned int? 

print_option_parameters uses PRId64, so here using PRId64 too to keep
output unchanged. In qemu-iotests, there are some tests having negative
size, like 'size=-1024', changing to PRIu64 will cause such iotests failed.

>  
> --  
> Eric Blake   eblake redhat com    +1-919-301-3266 
> Libvirt virtualization library http://libvirt.org 
>  
>  

Re: [Qemu-devel] [PATCH V26 03/32] QemuOpts: repurpose qemu_opts_print to replace print_option_parameters

[Stefan Hajnoczi]

On Tue, Apr 29, 2014 at 05:10:27PM +0800, Chunyan Liu wrote:
> Currently this function is not used anywhere. In later patches, it will
> replace print_option_parameters. To avoid print info changes, change
> qemu_opts_print from fprintf stderr to printf to keep consistent with
> print_option_parameters, remove last printf and print size/number with
> opt->value.uint instead of opt->str.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  util/qemu-option.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH V26 04/32] qapi: output def_value_str when query command line options

[Stefan Hajnoczi]

On Tue, Apr 29, 2014 at 05:10:28PM +0800, Chunyan Liu wrote:
> Change qapi interfaces to output the newly added def_value_str when querying
> command line options.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
> Changes to V25:
>   * update @default description in .json file
> 
>  qapi-schema.json   | 5 ++++-
>  qmp-commands.hx    | 2 ++
>  util/qemu-config.c | 4 ++++
>  3 files changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH V26 15/32] gluster.c: replace QEMUOptionParameter with QemuOpts

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On 04/29/2014 03:10 AM, Chunyan Liu wrote:
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
> Changes to V25:
>   * fix create_opts, OPT_STRING -> QEMU_OPT_STRING
> 
>  block/gluster.c | 81 ++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 42 insertions(+), 39 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V26 15/32] gluster.c: replace QEMUOptionParameter with QemuOpts

[Stefan Hajnoczi]

On Tue, Apr 29, 2014 at 05:10:39PM +0800, Chunyan Liu wrote:
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
> Changes to V25:
>   * fix create_opts, OPT_STRING -> QEMU_OPT_STRING
> 
>  block/gluster.c | 81 ++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 42 insertions(+), 39 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH V26 17/32] nfs.c: replace QEMUOptionParameter with QemuOpts

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

On 04/29/2014 03:10 AM, Chunyan Liu wrote:
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  block/nfs.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V26 17/32] nfs.c: replace QEMUOptionParameter with QemuOpts

[Stefan Hajnoczi]

On Tue, Apr 29, 2014 at 05:10:41PM +0800, Chunyan Liu wrote:
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  block/nfs.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH V26 27/32] vdi.c: replace QEMUOptionParameter with QemuOpts

[Stefan Hajnoczi]

On Tue, Apr 29, 2014 at 05:10:51PM +0800, Chunyan Liu wrote:
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  block/vdi.c | 73 +++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 35 insertions(+), 38 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH V26 00/32] replace QEMUOptionParameter with QemuOpts

[Stefan Hajnoczi]

On Tue, Apr 29, 2014 at 05:10:24PM +0800, Chunyan Liu wrote:
> This patch series is to replace QEMUOptionParameter with QemuOpts, so that only
> one Qemu Option structure is kept in QEMU code.
> 
> ---
> Changes to v25:
>   * split v25 2/31 (add def_value_str to QemuOptDesc) into two patches:
>     1st patch adds def_value_str, 2nd patch repurpose qemu_opts_print.
>   * update 4/32 qapi command line description.
>   * update 12/32 (change block layer to support both) according to
>     Eric's comments.
>   * small update to gluster.c
>   * rebase to latest code
> 
> All patches are also available from:
> https://github.com/chunyanliu/qemu/commits/QemuOpts

I looked through the comments from Leandro and Eric.  A respin is
necessary to address the test failures that Leandro found.  Please also
address the comments Eric made.

We're almost there now!

Re: [Qemu-devel] [PATCH V26 00/32] replace QEMUOptionParameter with QemuOpts

[Chun Yan Liu]

>>> On 5/6/2014 at 09:26 PM, in message
<20140506132615.GV15810@stefanha-thinkpad.redhat.com>, Stefan Hajnoczi
<stefanha@gmail.com> wrote: 
> On Tue, Apr 29, 2014 at 05:10:24PM +0800, Chunyan Liu wrote: 
> > This patch series is to replace QEMUOptionParameter with QemuOpts, so that  
> only 
> > one Qemu Option structure is kept in QEMU code. 
> >  
> > --- 
> > Changes to v25: 
> >   * split v25 2/31 (add def_value_str to QemuOptDesc) into two patches: 
> >     1st patch adds def_value_str, 2nd patch repurpose qemu_opts_print. 
> >   * update 4/32 qapi command line description. 
> >   * update 12/32 (change block layer to support both) according to 
> >     Eric's comments. 
> >   * small update to gluster.c 
> >   * rebase to latest code 
> >  
> > All patches are also available from: 
> > https://github.com/chunyanliu/qemu/commits/QemuOpts 
>  
> I looked through the comments from Leandro and Eric.  A respin is 
> necessary to address the test failures that Leandro found.  Please also 
> address the comments Eric made. 
>  
> We're almost there now! 
>  
>  

Hi, Stefan, Leandro & Eric,

Could you help to have a look at the new update? I've sent it about 10
days ago.  It addressed all Eric's comments in last version and two
memory free fix to address iotest issue. Hope it's the final work and
won't take you more time :-)

The thread has been cut into two, since the git-send-email died in the
middle at the 1st time sending. Please refer to:

[0~25]
http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg01098.html
[26~33]
http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg01135.html

Thanks!

-Chunyan

Re: [Qemu-devel] [PATCH V26 00/32] replace QEMUOptionParameter with QemuOpts

[Leandro Dorileo]

On Sun, May 18, 2014 at 09:02:45PM -0600, Chun Yan Liu wrote:
> 
> 
> >>> On 5/6/2014 at 09:26 PM, in message
> <20140506132615.GV15810@stefanha-thinkpad.redhat.com>, Stefan Hajnoczi
> <stefanha@gmail.com> wrote: 
> > On Tue, Apr 29, 2014 at 05:10:24PM +0800, Chunyan Liu wrote: 
> > > This patch series is to replace QEMUOptionParameter with QemuOpts, so that  
> > only 
> > > one Qemu Option structure is kept in QEMU code. 
> > >  
> > > --- 
> > > Changes to v25: 
> > >   * split v25 2/31 (add def_value_str to QemuOptDesc) into two patches: 
> > >     1st patch adds def_value_str, 2nd patch repurpose qemu_opts_print. 
> > >   * update 4/32 qapi command line description. 
> > >   * update 12/32 (change block layer to support both) according to 
> > >     Eric's comments. 
> > >   * small update to gluster.c 
> > >   * rebase to latest code 
> > >  
> > > All patches are also available from: 
> > > https://github.com/chunyanliu/qemu/commits/QemuOpts 
> >  
> > I looked through the comments from Leandro and Eric.  A respin is 
> > necessary to address the test failures that Leandro found.  Please also 
> > address the comments Eric made. 
> >  
> > We're almost there now! 
> >  
> >  
> 
> Hi, Stefan, Leandro & Eric,
> 
> Could you help to have a look at the new update? I've sent it about 10
> days ago.  It addressed all Eric's comments in last version and two
> memory free fix to address iotest issue. Hope it's the final work and
> won't take you more time :-)
> 
> The thread has been cut into two, since the git-send-email died in the
> middle at the 1st time sending. Please refer to:


I'll take a look at it, thanks.

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 00/32] replace QEMUOptionParameter with QemuOpts

[Chun Yan Liu]

>>> On 5/28/2014 at 10:42 AM, in message <53854CFA.E55 : 102 : 21807>, Chun Yan Liu
wrote: 

>  
>>>> On 5/20/2014 at 05:11 AM, in message 
> <20140519211122.GA2051@dorilex-lnv.MultilaserAP>, Leandro Dorileo 
> <l@dorileo.org> wrote:  
> > On Sun, May 18, 2014 at 09:02:45PM -0600, Chun Yan Liu wrote:  
>> >   
>> >   
>> > >>> On 5/6/2014 at 09:26 PM, in message  
>> > <20140506132615.GV15810@stefanha-thinkpad.redhat.com>, Stefan Hajnoczi  
>> > <stefanha@gmail.com> wrote:   
>> > > On Tue, Apr 29, 2014 at 05:10:24PM +0800, Chunyan Liu wrote:   
>> > > > This patch series is to replace QEMUOptionParameter with QemuOpts, so   
>> that    
>> > > only   
>> > > > one Qemu Option structure is kept in QEMU code.   
>> > > >    
>> > > > ---   
>> > > > Changes to v25:   
>> > > >   * split v25 2/31 (add def_value_str to QemuOptDesc) into two patches:   
>> > > >     1st patch adds def_value_str, 2nd patch repurpose qemu_opts_print.   
>> > > >   * update 4/32 qapi command line description.   
>> > > >   * update 12/32 (change block layer to support both) according to   
>> > > >     Eric's comments.   
>> > > >   * small update to gluster.c   
>> > > >   * rebase to latest code   
>> > > >    
>> > > > All patches are also available from:   
>> > > > https://github.com/chunyanliu/qemu/commits/QemuOpts   
>> > >    
>> > > I looked through the comments from Leandro and Eric.  A respin is   
>> > > necessary to address the test failures that Leandro found.  Please also   
>> > > address the comments Eric made.   
>> > >    
>> > > We're almost there now!   
>> > >    
>> > >    
>> >   
>> > Hi, Stefan, Leandro & Eric,  
>> >   
>> > Could you help to have a look at the new update? I've sent it about 10  
>> > days ago.  It addressed all Eric's comments in last version and two  
>> > memory free fix to address iotest issue. Hope it's the final work and  
>> > won't take you more time :-)  
>> >   
>> > The thread has been cut into two, since the git-send-email died in the  
>> > middle at the 1st time sending. Please refer to:  
>> > [0~25] 
>> > http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg01098.html 
>> > [26~33] 
>> > http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg01135.html 
>>   
>>   
>> I'll take a look at it, thanks.  
>   
> Hi, Leandro, is there any progress on this work? 
>  
> -Chunyan 

Hi, Stefan,

I saw Eric reviewed some patches of the new version last week. How about
the status of this work now?

- Chunyan