Re: [Qemu-devel] [PATCH v2] qemu-nbd: only send a limited number of errno codes on the wire

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Right now, NBD includes potentially platform-specific error values in
> the wire protocol.
>
> Luckily, most common error values are more or less universal: in
> particular, of all errno values <= 34 (up to ERANGE), they are all the
> same on supported platforms except for 11 (which is EAGAIN on Windows and
> Linux, but EDEADLK on Darwin and the *BSDs).  So, in order to guarantee
> some portability, only keep a handful of possible error codes and squash
> everything else to EINVAL.
>
> This patch defines a limited set of errno values that are valid for the
> NBD protocol, and specifies recommendations for what error to return
> in specific corner cases.  The set of errno values is roughly based on
> the errors listed in the read(2) and write(2) man pages, with some
> exceptions:
>
> - ENOMEM is added for servers that implement copy-on-write or other
>   formats that require dynamic allocation.
>
> - EDQUOT is not part of the universal set of errors; it can be changed
>   to ENOSPC on the wire format.

Makes sense.

> - EFBIG is part of the universal set of errors, but it is also changed
>   to ENOSPC because it is pretty similar to ENOSPC or EDQUOT.

Perhaps debatable, but I defer to your judgement.

> Incoming values will in general match system errno values, but not
> on the Hurd which has different errno values (they have a "subsystem
> code" equal to 0x10 in bits 24-31).  The Hurd is probably not something
> to which QEMU has been ported, but still do the right thing and
> reverse-map the NBD errno values to the system errno values.
>
> The corresponding patch to the NBD protocol description can be found at
> http://article.gmane.org/gmane.linux.drivers.nbd.general/3154.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>         v1->v2: use #defines and do not expect errnos to match
>                 NBD errnos for the sake of Hurd.  Reduced the list
>                 of errno values even more.  Better commit message
>                 explaining how the list was obtained.
>
>  nbd.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/nbd.c b/nbd.c
> index cb1b9bb..57d71b2 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -86,6 +86,54 @@
>  #define NBD_OPT_ABORT           (2)
>  #define NBD_OPT_LIST            (3)
>  
> +/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
> + * but only a limited set of errno values is specified in the protocol.
> + * Everything else is squashed to EINVAL.
> + */
> +#define NBD_EPERM      1
> +#define NBD_EIO        5
> +#define NBD_ENOMEM     12
> +#define NBD_EINVAL     22
> +#define NBD_ENOSPC     28
> +
> +static int system_errno_to_nbd_errno(int err)
> +{
> +    switch (err) {
> +    case EPERM:
> +        return NBD_EPERM;
> +    case EIO:
> +        return NBD_EIO;
> +    case ENOMEM:
> +        return NBD_ENOMEM;
> +#ifdef EDQUOT
> +    case EDQUOT:
> +#endif
> +    case EFBIG:
> +    case ENOSPC:
> +        return NBD_ENOSPC;
> +    case EINVAL:
> +    default:
> +        return NBD_EINVAL;
> +    }
> +}
> +
> +static int nbd_errno_to_system_errno(int err)
> +{
> +    switch (err) {
> +    case NBD_EPERM:
> +        return EPERM;
> +    case NBD_EIO:
> +        return EIO;
> +    case NBD_ENOMEM:
> +        return ENOMEM;
> +    case NBD_ENOSPC:
> +        return ENOSPC;
> +    case NBD_EINVAL:
> +    default:
> +        return EINVAL;
> +    }
> +}
> +

If we reach default, something's amiss, isn't it?  Worth logging
something then?

>  /* Definitions for opaque data types */
>  
>  typedef struct NBDRequest NBDRequest;
> @@ -856,6 +904,8 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply 
> *reply)
>      reply->error  = be32_to_cpup((uint32_t*)(buf + 4));
>      reply->handle = be64_to_cpup((uint64_t*)(buf + 8));
>  
> +    reply->error = nbd_errno_to_system_errno(reply->error);
> +
>      TRACE("Got reply: "
>            "{ magic = 0x%x, .error = %d, handle = %" PRIu64" }",
>            magic, reply->error, reply->handle);
> @@ -872,6 +922,8 @@ static ssize_t nbd_send_reply(int csock, struct nbd_reply 
> *reply)
>      uint8_t buf[NBD_REPLY_SIZE];
>      ssize_t ret;
>  
> +    reply->error = system_errno_to_nbd_errno(reply->error);
> +
>      /* Reply
>         [ 0 ..  3]    magic   (NBD_REPLY_MAGIC)
>         [ 4 ..  7]    error   (0 == no error)

Reviewed-by: Markus Armbruster <address@hidden>