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

[Paolo Bonzini <pbonzini@redhat.com>]

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