Re: [Qemu-devel] [PATCH 02/20] nbd: correctly propagate errors

[Fam Zheng <famz@redhat.com>]

On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Before:
>     $ ./qemu-io-old
>     qemu-io-old> open -r -o file.driver=nbd
>     one of path and host must be specified.
>     qemu-io-old: can't open device (null): Could not open image: Invalid argument
>     $ ./qemu-io-old
>     qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar
>     path and host may not be used at the same time.
>     qemu-io-old: can't open device (null): Could not open image: Invalid argument
> 
> After:
>     $ ./qemu-io
>     qemu-io> open -r -o file.driver=nbd
>     qemu-io: can't open device (null): one of path and host must be specified.
>     $ ./qemu-io
>     qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar
>     qemu-io: can't open device (null): path and host may not be used at the same time.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/nbd.c         | 34 ++++++++++++++++------------------
>  include/block/nbd.h |  1 -
>  nbd.c               | 12 ------------
>  3 files changed, 16 insertions(+), 31 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index fd89083..69f336b 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -188,19 +188,18 @@ out:
>      g_free(file);
>  }
>  
> -static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
> +static void nbd_config(BDRVNBDState *s, QDict *options, char **export,
> +                       Error **errp)
>  {
>      Error *local_err = NULL;
>  
>      if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
>          if (qdict_haskey(options, "path")) {
> -            qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
> -                          "be used at the same time.");
> +            error_setg(errp, "path and host may not be used at the same time.");
>          } else {
> -            qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host "
> -                          "must be specified.");
> +            error_setg(errp, "one of path and host must be specified.");
>          }
> -        return -EINVAL;
> +        return;
>      }
>  
>      s->client.is_unix = qdict_haskey(options, "path");
> @@ -209,9 +208,8 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
>  
>      qemu_opts_absorb_qdict(s->socket_opts, options, &local_err);
>      if (error_is_set(&local_err)) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> -        return -EINVAL;
> +        error_propagate(errp, local_err);
> +        return;
>      }
>  
>      if (!qemu_opt_get(s->socket_opts, "port")) {
> @@ -222,19 +220,17 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export)
>      if (*export) {
>          qdict_del(options, "export");
>      }
> -
> -    return 0;
>  }
>  
> -static int nbd_establish_connection(BlockDriverState *bs)
> +static int nbd_establish_connection(BlockDriverState *bs, Error **errp)
>  {
>      BDRVNBDState *s = bs->opaque;
>      int sock;
>  
>      if (s->client.is_unix) {
> -        sock = unix_socket_outgoing(qemu_opt_get(s->socket_opts, "path"));
> +        sock = unix_connect(qemu_opt_get(s->socket_opts, "path"), errp);

Why not use unix_connect_opts like below?

>      } else {
> -        sock = tcp_socket_outgoing_opts(s->socket_opts);
> +        sock = inet_connect_opts(s->socket_opts, errp, NULL, NULL);
>          if (sock >= 0) {
>              socket_set_nodelay(sock);
>          }
> @@ -255,17 +251,19 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
>      BDRVNBDState *s = bs->opaque;
>      char *export = NULL;
>      int result, sock;
> +    Error *local_err = NULL;
>  
>      /* Pop the config into our state object. Exit if invalid. */
> -    result = nbd_config(s, options, &export);
> -    if (result != 0) {
> -        return result;
> +    nbd_config(s, options, &export, &local_err);
> +    if (local_err) {

Isn't error_is_set() better here?

> +        error_propagate(errp, local_err);
> +        return -EINVAL;
>      }
>  
>      /* establish TCP connection, return error if it fails
>       * TODO: Configurable retry-until-timeout behaviour.
>       */
> -    sock = nbd_establish_connection(bs);
> +    sock = nbd_establish_connection(bs, errp);
>      if (sock < 0) {
>          return sock;
>      }

Thanks,

Fam