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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]