| Message ID | 1431078628-7856-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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.
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.
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>
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
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.
-----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-----
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
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)
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(+)