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

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

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

Excellent!

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

Oww!  That's going to hurd...

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

Thanks!