| Message ID | 1431012374-14113-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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)
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)
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)
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)
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!
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)
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
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)
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(+)