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

[Qemu-devel] [PATCH v2] 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 handful of possible error codes and squash
everything else to EINVAL.

This patch defines a limited set of errno values that are valid for the
NBD protocol, and specifies recommendations for what error to return
in specific corner cases.  The set of errno values is roughly based on
the errors listed in the read(2) and write(2) man pages, with some
exceptions:

- ENOMEM is added for servers that implement copy-on-write or other
  formats that require dynamic allocation.

- EDQUOT is not part of the universal set of errors; it can be changed
  to ENOSPC on the wire format.

- EFBIG is part of the universal set of errors, but it is also changed
  to ENOSPC because it is pretty similar to ENOSPC or EDQUOT.

Incoming values will in general match system errno values, but not
on the Hurd which has different errno values (they have a "subsystem
code" equal to 0x10 in bits 24-31).  The Hurd is probably not something
to which QEMU has been ported, but still do the right thing and
reverse-map the NBD errno values to the system errno values.

The corresponding patch to the NBD protocol description can be found at
http://article.gmane.org/gmane.linux.drivers.nbd.general/3154.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: use #defines and do not expect errnos to match
                NBD errnos for the sake of Hurd.  Reduced the list
                of errno values even more.  Better commit message
                explaining how the list was obtained.

 nbd.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/nbd.c b/nbd.c
index cb1b9bb..57d71b2 100644
--- a/nbd.c
+++ b/nbd.c
@@ -86,6 +86,54 @@
 #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.
+ */
+#define NBD_EPERM      1
+#define NBD_EIO        5
+#define NBD_ENOMEM     12
+#define NBD_EINVAL     22
+#define NBD_ENOSPC     28
+
+static int system_errno_to_nbd_errno(int err)
+{
+    switch (err) {
+    case EPERM:
+        return NBD_EPERM;
+    case EIO:
+        return NBD_EIO;
+    case ENOMEM:
+        return NBD_ENOMEM;
+#ifdef EDQUOT
+    case EDQUOT:
+#endif
+    case EFBIG:
+    case ENOSPC:
+        return NBD_ENOSPC;
+    case EINVAL:
+    default:
+        return NBD_EINVAL;
+    }
+}
+
+static int nbd_errno_to_system_errno(int err)
+{
+    switch (err) {
+    case NBD_EPERM:
+        return EPERM;
+    case NBD_EIO:
+        return EIO;
+    case NBD_ENOMEM:
+        return ENOMEM;
+    case NBD_ENOSPC:
+        return ENOSPC;
+    case NBD_EINVAL:
+    default:
+        return EINVAL;
+    }
+}
+
 /* Definitions for opaque data types */
 
 typedef struct NBDRequest NBDRequest;
@@ -856,6 +904,8 @@ 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));
 
+    reply->error = nbd_errno_to_system_errno(reply->error);
+
     TRACE("Got reply: "
           "{ magic = 0x%x, .error = %d, handle = %" PRIu64" }",
           magic, reply->error, reply->handle);
@@ -872,6 +922,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 v2] 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.
>
> 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 handful of possible error codes and squash
> everything else to EINVAL.
>
> This patch defines a limited set of errno values that are valid for the
> NBD protocol, and specifies recommendations for what error to return
> in specific corner cases.  The set of errno values is roughly based on
> the errors listed in the read(2) and write(2) man pages, with some
> exceptions:
>
> - ENOMEM is added for servers that implement copy-on-write or other
>   formats that require dynamic allocation.
>
> - EDQUOT is not part of the universal set of errors; it can be changed
>   to ENOSPC on the wire format.

Makes sense.

> - EFBIG is part of the universal set of errors, but it is also changed
>   to ENOSPC because it is pretty similar to ENOSPC or EDQUOT.

Perhaps debatable, but I defer to your judgement.

> Incoming values will in general match system errno values, but not
> on the Hurd which has different errno values (they have a "subsystem
> code" equal to 0x10 in bits 24-31).  The Hurd is probably not something
> to which QEMU has been ported, but still do the right thing and
> reverse-map the NBD errno values to the system errno values.
>
> The corresponding patch to the NBD protocol description can be found at
> http://article.gmane.org/gmane.linux.drivers.nbd.general/3154.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         v1->v2: use #defines and do not expect errnos to match
>                 NBD errnos for the sake of Hurd.  Reduced the list
>                 of errno values even more.  Better commit message
>                 explaining how the list was obtained.
>
>  nbd.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/nbd.c b/nbd.c
> index cb1b9bb..57d71b2 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -86,6 +86,54 @@
>  #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.
> + */
> +#define NBD_EPERM      1
> +#define NBD_EIO        5
> +#define NBD_ENOMEM     12
> +#define NBD_EINVAL     22
> +#define NBD_ENOSPC     28
> +
> +static int system_errno_to_nbd_errno(int err)
> +{
> +    switch (err) {
> +    case EPERM:
> +        return NBD_EPERM;
> +    case EIO:
> +        return NBD_EIO;
> +    case ENOMEM:
> +        return NBD_ENOMEM;
> +#ifdef EDQUOT
> +    case EDQUOT:
> +#endif
> +    case EFBIG:
> +    case ENOSPC:
> +        return NBD_ENOSPC;
> +    case EINVAL:
> +    default:
> +        return NBD_EINVAL;
> +    }
> +}
> +
> +static int nbd_errno_to_system_errno(int err)
> +{
> +    switch (err) {
> +    case NBD_EPERM:
> +        return EPERM;
> +    case NBD_EIO:
> +        return EIO;
> +    case NBD_ENOMEM:
> +        return ENOMEM;
> +    case NBD_ENOSPC:
> +        return ENOSPC;
> +    case NBD_EINVAL:
> +    default:
> +        return EINVAL;
> +    }
> +}
> +

If we reach default, something's amiss, isn't it?  Worth logging
something then?

>  /* Definitions for opaque data types */
>  
>  typedef struct NBDRequest NBDRequest;
> @@ -856,6 +904,8 @@ 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));
>  
> +    reply->error = nbd_errno_to_system_errno(reply->error);
> +
>      TRACE("Got reply: "
>            "{ magic = 0x%x, .error = %d, handle = %" PRIu64" }",
>            magic, reply->error, reply->handle);
> @@ -872,6 +922,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)

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Paolo Bonzini]

On 08/05/2015 12:12, Markus Armbruster wrote:
>> The corresponding patch to the NBD protocol description can be found at
>> http://article.gmane.org/gmane.linux.drivers.nbd.general/3154.

[...]

>> - EFBIG is part of the universal set of errors, but it is also changed
>>   to ENOSPC because it is pretty similar to ENOSPC or EDQUOT.
> 
> Perhaps debatable, but I defer to your judgement.

EFBIG is weird anyway, and requires you (or your parents) to ignore
SIGXFSZ.  A simpler protocol puts fewer requirements on the client.

(In fact, we probably should ignore SIGXFSZ in QEMU and treat EFBIG like
ENOSPC everywhere.  Should doesn't mean that it will get on anyone's
todo list or priority list...).

>> +static int nbd_errno_to_system_errno(int err)
>> +{
>> +    switch (err) {
>> +    case NBD_EPERM:
>> +        return EPERM;
>> +    case NBD_EIO:
>> +        return EIO;
>> +    case NBD_ENOMEM:
>> +        return ENOMEM;
>> +    case NBD_ENOSPC:
>> +        return ENOSPC;
>> +    case NBD_EINVAL:
>> +    default:
>> +        return EINVAL;
>> +    }
>> +}
>> +
> 
> If we reach default, something's amiss, isn't it?  Worth logging
> something then?

Not necessarily.  You could have an older NBD server on the other side,
and then the errno code might only be valid on another platform!

In fact, if the NBD server is qemu-nbd, the producer of the error could
be any driver in block/ for any old version of QEMU.  I would not be
surprised if an EPIPE or ENOTCONN sneaked in somehow.

And there are probably other NBD servers around.  CCing Rich Jones so
that he can fix nbdkit.

Paolo

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

[Richard W.M. Jones]

On Fri, May 08, 2015 at 12:27:59PM +0200, Paolo Bonzini wrote:
> And there are probably other NBD servers around.  CCing Rich Jones so
> that he can fix nbdkit.

Thanks.  FWIW currently nbdkit will send any arbitrary errno returned
by a system call, and it doesn't do any platform-specific encoding.

I've added this to the to-do list.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

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

[Eric Blake]

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

On 05/08/2015 04:27 AM, Paolo Bonzini wrote:
> 
> 
> On 08/05/2015 12:12, Markus Armbruster wrote:
>>> The corresponding patch to the NBD protocol description can be found at
>>> http://article.gmane.org/gmane.linux.drivers.nbd.general/3154.
> 
> [...]
> 
>>> - EFBIG is part of the universal set of errors, but it is also changed
>>>   to ENOSPC because it is pretty similar to ENOSPC or EDQUOT.
>>
>> Perhaps debatable, but I defer to your judgement.
> 
> EFBIG is weird anyway, and requires you (or your parents) to ignore
> SIGXFSZ.  A simpler protocol puts fewer requirements on the client.

Not only that, but if I understand correctly, the only way to trigger
SIGXFSZ or EFBIG is to compile your application to use a smaller off_t
than the maximum the system supports.  That is, it is impossible on
64-bit Linux, and on 32-bit Linux is it possible only when you use
32-bit off_t instead of 64-bit off_t.  But we are dealing with guest
disk images, and practically require 64-bit off_t to make it useful for
all but the oldest of guests.

> 
> (In fact, we probably should ignore SIGXFSZ in QEMU and treat EFBIG like
> ENOSPC everywhere.  Should doesn't mean that it will get on anyone's
> todo list or priority list...).

and if I'm right above, it wouldn't make a difference anyway.

-- 
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 v2] qemu-nbd: only send a limited number of errno codes on the wire

[Eric Blake]

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

On 05/08/2015 03:50 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).  So, in order to guarantee
> some portability, only keep a handful of possible error codes and squash
> everything else to EINVAL.
> 

> +static int system_errno_to_nbd_errno(int err)
> +{
> +    switch (err) {
> +    case EPERM:
> +        return NBD_EPERM;
> +    case EIO:
> +        return NBD_EIO;
> +    case ENOMEM:
> +        return NBD_ENOMEM;
> +#ifdef EDQUOT
> +    case EDQUOT:
> +#endif
> +    case EFBIG:
> +    case ENOSPC:
> +        return NBD_ENOSPC;
> +    case EINVAL:
> +    default:
> +        return NBD_EINVAL;
> +    }

Do we also want to handle "case 0: return 0;" on either conversion, or
even "case 0: abort();" to ensure that callers are using these helpers
correctly?

But I can also live with it as is;
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v2] qemu-nbd: only send a limited number of errno codes on the wire

[Paolo Bonzini]

On 08/05/2015 14:40, Eric Blake wrote:
> On 05/08/2015 03:50 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).  So, in order to guarantee
>> some portability, only keep a handful of possible error codes and squash
>> everything else to EINVAL.
>>
> 
>> +static int system_errno_to_nbd_errno(int err)
>> +{
>> +    switch (err) {
>> +    case EPERM:
>> +        return NBD_EPERM;
>> +    case EIO:
>> +        return NBD_EIO;
>> +    case ENOMEM:
>> +        return NBD_ENOMEM;
>> +#ifdef EDQUOT
>> +    case EDQUOT:
>> +#endif
>> +    case EFBIG:
>> +    case ENOSPC:
>> +        return NBD_ENOSPC;
>> +    case EINVAL:
>> +    default:
>> +        return NBD_EINVAL;
>> +    }
> 
> Do we also want to handle "case 0: return 0;" on either conversion, or
> even "case 0: abort();" to ensure that callers are using these helpers
> correctly?

Yes, it's much better that way.

Paolo

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

[Eric Blake]

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

On 05/08/2015 06:41 AM, Paolo Bonzini wrote:
> 
> 
> On 08/05/2015 14:40, Eric Blake wrote:
>> On 05/08/2015 03:50 AM, Paolo Bonzini wrote:
>>> Right now, NBD includes potentially platform-specific error values in
>>> the wire protocol.
>>>

>>
>> Do we also want to handle "case 0: return 0;" on either conversion, or
>> even "case 0: abort();" to ensure that callers are using these helpers
>> correctly?
> 
> Yes, it's much better that way.

Thinking about it a bit more: abort() is fine on the sending side, to
ensure we aren't putting garbage on the wire; but abort() on the
receiving side is a bit risky (we should be handling a corrupted
incoming stream gracefully - a malicious sender should not be able to
crash us).  Of course, once we've detected a corrupted incoming stream,
we can't do much for the block device the stream was supposed to
represent (perhaps treat it as EIO and declare the device dead), but
that's still better than aborting.

-- 
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 v2] qemu-nbd: only send a limited number of errno codes on the wire

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 08/05/2015 15:02, Eric Blake wrote:
>>>>> Do we also want to handle "case 0: return 0;" on either
>>>>> conversion, or even "case 0: abort();" to ensure that
>>>>> callers are using these helpers correctly?
>>> 
>>> Yes, it's much better that way.
> Thinking about it a bit more: abort() is fine on the sending side,
> to ensure we aren't putting garbage on the wire; but abort() on
> the receiving side is a bit risky (we should be handling a
> corrupted incoming stream gracefully - a malicious sender should
> not be able to crash us).  Of course, once we've detected a
> corrupted incoming stream, we can't do much for the block device
> the stream was supposed to represent (perhaps treat it as EIO and
> declare the device dead), but that's still better than aborting.

I've included "case 0: return 0;" in the pull request.

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJVTLRxAAoJEL/70l94x66DiU0IAIc9c7XNRGkOzvx8Dr/euTZh
urx9OVG4Mwpust8KDmquuBJUv18Xu+omh9AxWOkF5ChZUGBwVxyt0N4sKPreVXK2
zJgObt+5cV4o1FXMb3QcFn1CD7s6+8V8T2QukGTviCsIwRaovwpBQAMuT4N5aJWY
BTkCE8GiGTVYiWhV+Uz3UML33j5mXJ4WM2LP11ndsKZpFNMvezwo9iyvQe28EqrI
Cj6xz+pLbnOtTu3/Kdf1SMA4FK9loH7/y6fth833TIBE/OIyY+PEtNKoq7TJzc3i
/S58dVjXNZAez1Bf0Hb9rMCKsGg/MSv7mGPU5IsLQSKgZ0i154ZzJFxifkXnzYU=
=fZNP
-----END PGP SIGNATURE-----

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

[Thomas Schwinge]

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

Hi!

With my GNU Hurd hat on:

On Fri,  8 May 2015 11:50:28 +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Right now, NBD includes potentially platform-specific error values in
> the wire protocol.

> This patch [...]

> Incoming values will in general match system errno values, but not
> on the Hurd which has different errno values (they have a "subsystem
> code" equal to 0x10 in bits 24-31).  The Hurd is probably not something
> to which QEMU has been ported, but still do the right thing and
> reverse-map the NBD errno values to the system errno values.
> 
> The corresponding patch to the NBD protocol description can be found at
> http://article.gmane.org/gmane.linux.drivers.nbd.general/3154.

Thank you!  I added this information/links to
<http://darnassus.sceen.net/~hurd-web/hurd/libstore/nbd_store/>.


Grüße,
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]