[Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export

[Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export

[Paolo Bonzini]

This is consistent with the handling of writes.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/nbd.c b/nbd.c
index cb1b9bb..eea8c51 100644
--- a/nbd.c
+++ b/nbd.c
@@ -1325,6 +1325,12 @@ static void nbd_trip(void *opaque)
         break;
     case NBD_CMD_TRIM:
         TRACE("Request type is TRIM");
+        if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
+            TRACE("Server is read-only, return error");
+            reply.error = EROFS;
+            goto error_reply;
+        }
+
         ret = blk_co_discard(exp->blk, (request.from + exp->dev_offset)
                                        / BDRV_SECTOR_SIZE,
                              request.len / BDRV_SECTOR_SIZE);
-- 
2.3.5

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

[Paolo Bonzini]

Right now, NBD includes potentially platform-specific error values in
the wire protocol.

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

So, in order to guarantee some portability, only keep a dozen
possible error codes and squash everything else to EINVAL.

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.
+ */
+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;
+    }
+}
+
 /* 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);
+
     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)
-- 
2.3.5

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 952 bytes --]

On 05/07/2015 09:26 AM, Paolo Bonzini wrote:
> Right now, NBD includes potentially platform-specific error values in
> the wire protocol.
> 
> 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).

Well, this is not true on GNU Hurd, but to my knowledge no one has tried
to compile qemu on Hurd.

>  
> +    /* NBD errors should be universally equal to the corresponding
> +     * errno values, check it here.
> +     */
> +    QEMU_BUILD_BUG_ON(EPERM != 1);

And this is (probably not the first place) where qemu compilation would
fail on Hurd.  At any rate, I see no reason to hold up the patch on that
ground.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Paolo Bonzini]

On 07/05/2015 19:01, Eric Blake wrote:
>> >  
>> > +    /* NBD errors should be universally equal to the corresponding
>> > +     * errno values, check it here.
>> > +     */
>> > +    QEMU_BUILD_BUG_ON(EPERM != 1);
> And this is (probably not the first place) where qemu compilation would
> fail on Hurd.  At any rate, I see no reason to hold up the patch on that
> ground.

Well, I can certainly add a backwards mapping.

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Right now, NBD includes potentially platform-specific error values in
> the wire protocol.

Design flaw.

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

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

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)

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

[Paolo Bonzini]

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)

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

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

Good.

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

Okay, I just succeeded in demonstrating my ignorance %-)

>                                          But I agree that the "right"
> fix without backwards-compatibility would be to make the errors
> something like 0x80000000 to 0x80000004.
>
> Paolo

Two more remarks below.

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

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

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

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

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

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)

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@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!

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

[Max Reitz]

On 07.05.2015 17:26, Paolo Bonzini wrote:
> Right now, NBD includes potentially platform-specific error values in
> the wire protocol.
>
> 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).
>
> So, in order to guarantee some portability, only keep a dozen
> possible error codes and squash everything else to EINVAL.
>
> 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.
> + */
> +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;
> +    }
> +}
> +
>   /* 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);
> +

Why no nbd_errno_to_system_errno() function?

Max

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

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

[Max Reitz]

On 08.05.2015 14:45, Max Reitz wrote:
> On 07.05.2015 17:26, Paolo Bonzini wrote:
>> Right now, NBD includes potentially platform-specific error values in
>> the wire protocol.
>>
>> 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).
>>
>> So, in order to guarantee some portability, only keep a dozen
>> possible error codes and squash everything else to EINVAL.
>>
>> 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.
>> + */
>> +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;
>> +    }
>> +}
>> +
>>   /* 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);
>> +
>
> Why no nbd_errno_to_system_errno() function?

Oops, I missed v2. Sorry.

Max

Re: [Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export

[Max Reitz]

On 07.05.2015 17:26, Paolo Bonzini wrote:
> This is consistent with the handling of writes.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   nbd.c | 6 ++++++
>   1 file changed, 6 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>