Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL

[Paolo Bonzini <pbonzini@redhat.com>]

On 25/02/2015 19:08, Max Reitz wrote:
> Just returning -EINVAL for everything is bad. -EIO is often better, and
> sometimes there is an even more fitting value.

Propagating the return value from write_sync is uglier, but it is even
better in terms of returned value.

Paolo

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  nbd.c | 49 +++++++++++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index b4776ce..d2eb1c5 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -238,22 +238,22 @@ static int nbd_send_rep(int csock, uint32_t type, uint32_t opt)
>      magic = cpu_to_be64(NBD_REP_MAGIC);
>      if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>          LOG("write failed (rep magic)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      opt = cpu_to_be32(opt);
>      if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
>          LOG("write failed (rep opt)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      type = cpu_to_be32(type);
>      if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
>          LOG("write failed (rep type)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      len = cpu_to_be32(0);
>      if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>          LOG("write failed (rep data length)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> @@ -267,31 +267,31 @@ static int nbd_send_rep_list(int csock, NBDExport *exp)
>      magic = cpu_to_be64(NBD_REP_MAGIC);
>      if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>          LOG("write failed (magic)");
> -        return -EINVAL;
> +        return -EIO;
>       }
>      opt = cpu_to_be32(NBD_OPT_LIST);
>      if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
>          LOG("write failed (opt)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      type = cpu_to_be32(NBD_REP_SERVER);
>      if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
>          LOG("write failed (reply type)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      len = cpu_to_be32(name_len + sizeof(len));
>      if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>          LOG("write failed (length)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      len = cpu_to_be32(name_len);
>      if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>          LOG("write failed (length)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      if (write_sync(csock, exp->name, name_len) != name_len) {
>          LOG("write failed (buffer)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> @@ -309,7 +309,7 @@ static int nbd_handle_list(NBDClient *client, uint32_t length)
>      /* For each export, send a NBD_REP_SERVER reply. */
>      QTAILQ_FOREACH(exp, &exports, next) {
>          if (nbd_send_rep_list(csock, exp)) {
> -            return -EINVAL;
> +            return -EIO;
>          }
>      }
>      /* Finish with a NBD_REP_ACK. */
> @@ -331,6 +331,7 @@ static int nbd_handle_export_name(NBDClient *client, uint32_t length)
>      }
>      if (read_sync(csock, name, length) != length) {
>          LOG("read failed");
> +        rc = -EIO;
>          goto fail;
>      }
>      name[length] = '\0';
> @@ -376,22 +377,22 @@ static int nbd_receive_options(NBDClient *client)
>  
>          if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>              LOG("read failed");
> -            return -EINVAL;
> +            return -EIO;
>          }
>          TRACE("Checking opts magic");
>          if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
>              LOG("Bad magic received");
> -            return -EINVAL;
> +            return -EIO;
>          }
>  
>          if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
>              LOG("read failed");
> -            return -EINVAL;
> +            return -EIO;
>          }
>  
>          if (read_sync(csock, &length, sizeof(length)) != sizeof(length)) {
>              LOG("read failed");
> -            return -EINVAL;
> +            return -EIO;
>          }
>          length = be32_to_cpu(length);
>  
> @@ -404,7 +405,7 @@ static int nbd_receive_options(NBDClient *client)
>              break;
>  
>          case NBD_OPT_ABORT:
> -            return -EINVAL;
> +            return -ECONNABORTED;
>  
>          case NBD_OPT_EXPORT_NAME:
>              return nbd_handle_export_name(client, length);
> @@ -446,7 +447,7 @@ static int nbd_send_negotiate(NBDClient *client)
>       */
>  
>      qemu_set_block(csock);
> -    rc = -EINVAL;
> +    rc = -EIO;
>  
>      TRACE("Beginning negotiation.");
>      memset(buf, 0, sizeof(buf));
> @@ -503,7 +504,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
>  
>      TRACE("Receiving negotiation.");
>  
> -    rc = -EINVAL;
> +    rc = -EIO;
>  
>      if (read_sync(csock, buf, 8) != 8) {
>          error_setg(errp, "Failed to read data");
> @@ -752,7 +753,7 @@ ssize_t nbd_send_request(int csock, struct nbd_request *request)
>  
>      if (ret != sizeof(buf)) {
>          LOG("writing to socket failed");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> @@ -770,7 +771,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
>  
>      if (ret != sizeof(buf)) {
>          LOG("read failed");
> -        return -EINVAL;
> +        return -EIO;
>      }
>  
>      /* Request
> @@ -793,7 +794,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
>  
>      if (magic != NBD_REQUEST_MAGIC) {
>          LOG("invalid magic (got 0x%x)", magic);
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> @@ -811,7 +812,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
>  
>      if (ret != sizeof(buf)) {
>          LOG("read failed");
> -        return -EINVAL;
> +        return -EIO;
>      }
>  
>      /* Reply
> @@ -830,7 +831,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
>  
>      if (magic != NBD_REPLY_MAGIC) {
>          LOG("invalid magic (got 0x%x)", magic);
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> @@ -858,7 +859,7 @@ static ssize_t nbd_send_reply(int csock, struct nbd_reply *reply)
>  
>      if (ret != sizeof(buf)) {
>          LOG("writing to socket failed");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
>