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)
