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

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Right now, NBD includes potentially platform-specific error values in
> the wire protocol.

Design flaw.

> 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).

Can EAGAIN or EDEADLK happen?  "I don't know" is an acceptable answer :)

> So, in order to guarantee some portability, only keep a dozen
> possible error codes and squash everything else to EINVAL.

Ugh.  I guess it'll do.

Cleaner solution: Fix the protocol to transmit "EPERM", "EIO", ... in
addition to 1, 5, ...

If backward compatibility is not an issue: s/in addition to/instead of/.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  nbd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git a/nbd.c b/nbd.c
> index eea8c51..1ad5b66 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -86,6 +86,37 @@
>  #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.
> + */

Is the protocol defined anywhere?

> +static int system_errno_to_nbd_errno(int err)
> +{
> +    switch (err) {
> +    case EPERM:
> +        return 1;
> +    case EIO:
> +        return 5;
> +    case ENXIO:
> +        return 6;
> +    case E2BIG:
> +        return 7;
> +    case ENOMEM:
> +        return 12;
> +    case EACCES:
> +        return 13;
> +    case EFBIG:
> +        return 27;
> +    case ENOSPC:
> +        return 28;
> +    case EROFS:
> +        return 30;
> +    case EINVAL:
> +    default:
> +        return 22;
> +    }
> +}
> +

This maps recognized OS errnos to NBD errnos.  The latter are literals.

>  /* Definitions for opaque data types */
>  
>  typedef struct NBDRequest NBDRequest;
> @@ -856,6 +887,20 @@ 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));
>  
> +    /* NBD errors should be universally equal to the corresponding
> +     * errno values, check it here.
> +     */
> +    QEMU_BUILD_BUG_ON(EPERM != 1);
> +    QEMU_BUILD_BUG_ON(EIO != 5);
> +    QEMU_BUILD_BUG_ON(ENXIO != 6);
> +    QEMU_BUILD_BUG_ON(E2BIG != 7);
> +    QEMU_BUILD_BUG_ON(ENOMEM != 12);
> +    QEMU_BUILD_BUG_ON(EACCES != 13);
> +    QEMU_BUILD_BUG_ON(EINVAL != 22);
> +    QEMU_BUILD_BUG_ON(EFBIG != 27);
> +    QEMU_BUILD_BUG_ON(ENOSPC != 28);
> +    QEMU_BUILD_BUG_ON(EROFS != 30);
> +

This checks that the mapping above is the identify function for all the
recognized NBD errnos.  Why is that necessary?

Same literals as above.  Violates DRY.  I don't mind all that much, but
wonder whether we could at least do the checking next to
system_errno_to_nbd_errno().

>      TRACE("Got reply: "
>            "{ magic = 0x%x, .error = %d, handle = %" PRIu64" }",
>            magic, reply->error, reply->handle);
> @@ -872,6 +917,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)