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:

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

Good.

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

Okay, I just succeeded in demonstrating my ignorance %-)

>                                          But I agree that the "right"
> fix without backwards-compatibility would be to make the errors
> something like 0x80000000 to 0x80000004.
>
> Paolo

Two more remarks below.

>> 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?

https://github.com/yoe/nbd/blob/master/doc/proto.txt

All it has on the error code is this paragraph:

    The reply contains three fields: a 32 bit magic number ('magic'), a
    32 bit error code ('error'; 0, unless an error occurred in which
    case it is the errno of the error on the server side), and the same
    64 bit handle that the corresponding request had in its 'handle'
    field.  In case the reply is sent in response to a read request and
    the error field is 0 (zero), the reply header is immediately
    followed by request.len bytes of data.

Could you update it to document the errno compatibility issues, and
recommended practice (i.e. this patch's)?

>>> +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?

Still curious.  Explain, and earn my R-by :)

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

Could we?

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