On 08/05/2015 08:45, Markus Armbruster wrote: >> 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 :)
No. Even stuff like EFAULT would be a bug. >> 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, ... Why? It's a binary protocol after all. But I agree that the "right" fix without backwards-compatibility would be to make the errors something like 0x80000000 to 0x80000004. Paolo > 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)