Paolo Bonzini <pbonz...@redhat.com> 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!
