Paolo Bonzini <pbonz...@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 <pbonz...@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)
