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

[Paolo Bonzini]

On 08/05/2015 11:32, Markus Armbruster wrote:
>>>> +/* 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)?

Yes, I've sent a patch yesterday.

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

Because block/nbd.c expects host errnos, and I was too lazy to add a
switch and a nbd_errno_to_system_errno function.  But Eric pointed out
that Hurd has weird errnos (the low bits match, but there's a 0x10
subsystem code in bits 24-31) so I'll add it.

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

Yes, s/could/should/.  Also, should have added RFC to the patch.

Paolo

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