Re: [Qemu-devel] [PATCH v1 1/8] net: Move error reporting from net_init_client/netdev to the calling site

[Paolo Bonzini]

On 19/02/2018 10:15, Thomas Huth wrote:
> It looks strange that net_init_client() and net_init_netdev() both
> take an "Error **errp" parameter, but then do the error reporting
> with "error_report_err(local_err)" on their own. Let's move the
> error reporting to the calling site instead to simplify this code
> a little bit.
> 
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  include/net/net.h |  2 +-
>  net/net.c         | 29 +++++------------------------
>  vl.c              |  3 ++-
>  3 files changed, 8 insertions(+), 26 deletions(-)
> 
> diff --git a/include/net/net.h b/include/net/net.h
> index 3fc48e4..bdd4d9f 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -206,7 +206,7 @@ extern const char *legacy_bootp_filename;
>  
>  int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp);
>  int net_client_parse(QemuOptsList *opts_list, const char *str);
> -int net_init_clients(void);
> +int net_init_clients(Error **errp);
>  void net_check_clients(void);
>  void net_cleanup(void);
>  void hmp_host_net_add(Monitor *mon, const QDict *qdict);
> diff --git a/net/net.c b/net/net.c
> index 7d42925..e213a61 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1520,46 +1520,27 @@ void net_check_clients(void)
>  
>  static int net_init_client(void *dummy, QemuOpts *opts, Error **errp)
>  {
> -    Error *local_err = NULL;
> -
> -    net_client_init(opts, false, &local_err);
> -    if (local_err) {
> -        error_report_err(local_err);
> -        return -1;
> -    }
> -
> -    return 0;
> +    return net_client_init(opts, false, errp);
>  }
>  
>  static int net_init_netdev(void *dummy, QemuOpts *opts, Error **errp)
>  {
> -    Error *local_err = NULL;
> -    int ret;
> -
> -    ret = net_client_init(opts, true, &local_err);
> -    if (local_err) {
> -        error_report_err(local_err);
> -        return -1;
> -    }
> -
> -    return ret;
> +    return net_client_init(opts, true, errp);
>  }
>  
> -int net_init_clients(void)
> +int net_init_clients(Error **errp)
>  {
> -    QemuOptsList *net = qemu_find_opts("net");
> -
>      net_change_state_entry =
>          qemu_add_vm_change_state_handler(net_vm_change_state_handler, NULL);
>  
>      QTAILQ_INIT(&net_clients);
>  
>      if (qemu_opts_foreach(qemu_find_opts("netdev"),
> -                          net_init_netdev, NULL, NULL)) {
> +                          net_init_netdev, NULL, errp)) {
>          return -1;
>      }
>  
> -    if (qemu_opts_foreach(net, net_init_client, NULL, NULL)) {
> +    if (qemu_opts_foreach(qemu_find_opts("net"), net_init_client, NULL, 
> errp)) {
>          return -1;
>      }
>  
> diff --git a/vl.c b/vl.c
> index 7a5554b..7e95711 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4487,7 +4487,8 @@ int main(int argc, char **argv, char **envp)
>  
>      colo_info_init();
>  
> -    if (net_init_clients() < 0) {
> +    if (net_init_clients(&err) < 0) {
> +        error_report_err(err);
>          exit(1);
>      }
>  
> 

Reviewed-by: Paolo Bonzini <address@hidden>