Re: [Qemu-devel] [PATCH 8/9] QemuOpts: Convert qemu_opt_foreach() to Error

[Eric Blake]

On 05/28/2015 06:21 AM, Markus Armbruster wrote:
> Retain the function value for now, to permit selective conversion of
> its callers.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  include/qemu/option.h | 7 +++++--
>  net/vhost-user.c      | 8 +++++---
>  qdev-monitor.c        | 5 +++--
>  ui/spice-core.c       | 5 +++--
>  util/qemu-config.c    | 5 +++--
>  util/qemu-option.c    | 8 +++++---
>  vl.c                  | 9 +++++----
>  7 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index a3cf4c1..ac0e43b 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -100,8 +100,11 @@ void qemu_opt_set_bool(QemuOpts *opts, const char *name, 
> bool val,
>                         Error **errp);
>  void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
>                           Error **errp);
> -typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void 
> *opaque);
> -int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque);
> +typedef int (*qemu_opt_loopfunc)(void *opaque,
> +                                 const char *name, const char *value,
> +                                 Error **errp);

Again, justification for reordering callback parameter ordering might be
nice to mention in the commit message, but the code itself is correct, so:

Reviewed-by: Eric Blake <address@hidden>


> +++ b/util/qemu-option.c
> @@ -597,17 +597,19 @@ void qemu_opt_set_number(QemuOpts *opts, const char 
> *name, int64_t val,
>  }
>  
>  /**
> - * For each member of @opts, call @func(name, value, @opaque).
> + * For each member of @opts, call @func(@opaque, name, value, @errp).
> + * @func() may store an Error through @errp, but must return non-zero then.
>   * When @func() returns non-zero, break the loop and return that value.
>   * Return zero when the loop completes.
>   */
> -int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque)
> +int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
> +                     Error **errp)
>  {
>      QemuOpt *opt;
>      int rc;
>  
>      QTAILQ_FOREACH(opt, &opts->head, next) {
> -        rc = func(opt->name, opt->str, opaque);
> +        rc = func(opaque, opt->name, opt->str, errp);
>          if (rc) {
>              return rc;
>          }

As in my earlier review, wouldn't it be better to add:

assert(!*errp)

at this point, to ensure we meet the contract?

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

signature.asc
Description: OpenPGP digital signature