All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export
@ 2015-05-07 15:26 Paolo Bonzini
  2015-05-07 15:26 ` [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire Paolo Bonzini
  2015-05-08 12:43 ` [Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export Max Reitz
  0 siblings, 2 replies; 12+ messages in thread
From: Paolo Bonzini @ 2015-05-07 15:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, mreitz

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire
  2015-05-07 15:26 [Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export Paolo Bonzini
@ 2015-05-07 15:26 ` Paolo Bonzini
  2015-05-07 17:01   ` Eric Blake
                     ` (2 more replies)
  2015-05-08 12:43 ` [Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export Max Reitz
  1 sibling, 3 replies; 12+ messages in thread
From: Paolo Bonzini @ 2015-05-07 15:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, mreitz

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire
  2015-05-07 15:26 ` [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire Paolo Bonzini
@ 2015-05-07 17:01   ` Eric Blake
  2015-05-07 17:06     ` Paolo Bonzini
  2015-05-08  6:45   ` Markus Armbruster
  2015-05-08 12:45   ` Max Reitz
  2 siblings, 1 reply; 12+ messages in thread
From: Eric Blake @ 2015-05-07 17:01 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: kwolf, mreitz

[-- 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 --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire
  2015-05-07 17:01   ` Eric Blake
@ 2015-05-07 17:06     ` Paolo Bonzini
  0 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2015-05-07 17:06 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, mreitz



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire
  2015-05-07 15:26 ` [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire Paolo Bonzini
  2015-05-07 17:01   ` Eric Blake
@ 2015-05-08  6:45   ` Markus Armbruster
  2015-05-08  8:30     ` Paolo Bonzini
  2015-05-08 12:45   ` Max Reitz
  2 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2015-05-08  6:45 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kwolf, qemu-devel, mreitz

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)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire
  2015-05-08  6:45   ` Markus Armbruster
@ 2015-05-08  8:30     ` Paolo Bonzini
  2015-05-08  9:32       ` Markus Armbruster
  0 siblings, 1 reply; 12+ messages in thread
From: Paolo Bonzini @ 2015-05-08  8:30 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: kwolf, qemu-devel, mreitz



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)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire
  2015-05-08  8:30     ` Paolo Bonzini
@ 2015-05-08  9:32       ` Markus Armbruster
  2015-05-08  9:40         ` Paolo Bonzini
  0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2015-05-08  9:32 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kwolf, qemu-devel, mreitz

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)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire
  2015-05-08  9:32       ` Markus Armbruster
@ 2015-05-08  9:40         ` Paolo Bonzini
  2015-05-08 10:04           ` Markus Armbruster
  0 siblings, 1 reply; 12+ messages in thread
From: Paolo Bonzini @ 2015-05-08  9:40 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: kwolf, qemu-devel, mreitz



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)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire
  2015-05-08  9:40         ` Paolo Bonzini
@ 2015-05-08 10:04           ` Markus Armbruster
  0 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2015-05-08 10:04 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kwolf, qemu-devel, mreitz

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!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export
  2015-05-07 15:26 [Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export Paolo Bonzini
  2015-05-07 15:26 ` [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire Paolo Bonzini
@ 2015-05-08 12:43 ` Max Reitz
  1 sibling, 0 replies; 12+ messages in thread
From: Max Reitz @ 2015-05-08 12:43 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: kwolf

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire
  2015-05-07 15:26 ` [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire Paolo Bonzini
  2015-05-07 17:01   ` Eric Blake
  2015-05-08  6:45   ` Markus Armbruster
@ 2015-05-08 12:45   ` Max Reitz
  2015-05-08 13:01     ` Max Reitz
  2 siblings, 1 reply; 12+ messages in thread
From: Max Reitz @ 2015-05-08 12:45 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: kwolf

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)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire
  2015-05-08 12:45   ` Max Reitz
@ 2015-05-08 13:01     ` Max Reitz
  0 siblings, 0 replies; 12+ messages in thread
From: Max Reitz @ 2015-05-08 13:01 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: kwolf

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-05-08 13:02 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-07 15:26 [Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export Paolo Bonzini
2015-05-07 15:26 ` [Qemu-devel] [PATCH] qemu-nbd: only send a limited number of errno codes on the wire Paolo Bonzini
2015-05-07 17:01   ` Eric Blake
2015-05-07 17:06     ` Paolo Bonzini
2015-05-08  6:45   ` Markus Armbruster
2015-05-08  8:30     ` Paolo Bonzini
2015-05-08  9:32       ` Markus Armbruster
2015-05-08  9:40         ` Paolo Bonzini
2015-05-08 10:04           ` Markus Armbruster
2015-05-08 12:45   ` Max Reitz
2015-05-08 13:01     ` Max Reitz
2015-05-08 12:43 ` [Qemu-devel] [PATCH] qemu-nbd: return EROFS for discard on a read-only export Max Reitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.