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

[Jeff Cody]

On Tue, Feb 11, 2014 at 06:03:35PM +0100, 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 <address@hidden>


And this is the diff for 051.out for this patch (patch 2):


diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 3e8d962..7de1870 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -231,8 +231,7 @@ Testing: -drive driver=file
 QEMU_PROG: -drive driver=file: could not open disk image ide0-hd0: The 'file' 
block driver requires a file name
 
 Testing: -drive driver=nbd
-QEMU_PROG: -drive driver=nbd: one of path and host must be specified.
-QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: Could not 
open image: Invalid argument
+QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: one of path 
and host must be specified.
 
 Testing: -drive driver=raw
 QEMU_PROG: -drive driver=raw: could not open disk image ide0-hd0: Can't use 
'raw' as a block driver for the protocol level
@@ -241,8 +240,7 @@ Testing: -drive file.driver=file
 QEMU_PROG: -drive file.driver=file: could not open disk image ide0-hd0: The 
'file' block driver requires a file name
 
 Testing: -drive file.driver=nbd
-QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified.
-QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: Could 
not open image: Invalid argument
+QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: one of 
path and host must be specified.
 
 Testing: -drive file.driver=raw
 QEMU_PROG: -drive file.driver=raw: could not open disk image ide0-hd0: Can't 
use 'raw' as a block driver for the protocol level



> ---
>  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_opts(s->socket_opts, errp, NULL, NULL);
>      } 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) {
> +        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;
>      }
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index c90f5e4..e10ab82 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -64,7 +64,6 @@ enum {
>  ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
>  int tcp_socket_incoming(const char *address, uint16_t port);
>  int tcp_socket_incoming_spec(const char *address_and_port);
> -int tcp_socket_outgoing_opts(QemuOpts *opts);
>  int unix_socket_outgoing(const char *path);
>  int unix_socket_incoming(const char *path);
>  
> diff --git a/nbd.c b/nbd.c
> index 030f56b..17ca95b 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -199,18 +199,6 @@ static void combine_addr(char *buf, size_t len, const 
> char* address,
>      }
>  }
>  
> -int tcp_socket_outgoing_opts(QemuOpts *opts)
> -{
> -    Error *local_err = NULL;
> -    int fd = inet_connect_opts(opts, &local_err, NULL, NULL);
> -    if (local_err != NULL) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> -    }
> -
> -    return fd;
> -}
> -
>  int tcp_socket_incoming(const char *address, uint16_t port)
>  {
>      char address_and_port[128];
> -- 
> 1.8.5.3
> 
> 
>