Re: [Qemu-devel] [PATCH v2] qemu-nbd: only send a limited number of errno codes on the wire
Hi! With my GNU Hurd hat on: On Fri, 8 May 2015 11:50:28 +0200, Paolo Bonzini pbonz...@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 signature.asc Description: PGP signature
[Qemu-devel] [PATCH v2] qemu-nbd: only send a limited number of errno codes on the wire
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 pbonz...@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_EIO5
+#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
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
Paolo Bonzini pbonz...@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 pbonz...@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_EIO5
+#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 arm...@redhat.com
Re: [Qemu-devel] [PATCH v2] qemu-nbd: only send a limited number of errno codes on the wire
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
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
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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2] qemu-nbd: only send a limited number of errno codes on the wire
-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
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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2] qemu-nbd: only send a limited number of errno codes on the wire
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 ebl...@redhat.com
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
