[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno c
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire |
Date: |
Fri, 08 May 2015 10:30:02 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
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 <address@hidden>
>> ---
>> 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)
Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire, Max Reitz, 2015/05/08
Re: [Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export, Max Reitz, 2015/05/08