[2/3] NBD proto: document additional error conditions
diff mbox

Message ID 1459161798-32120-3-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev March 28, 2016, 10:43 a.m. UTC
From: Pavel Borzenkov <pborzenkov@virtuozzo.com>

It is unclear what the behaviour of a server should be if it receives
an unknown command. Similar uncertainty exists for command flags.

Make it explicit that the server should return EINVAL in all such cases.

Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Wouter Verhelst <w@uter.be>
CC: Eric Blake <eblake@redhat.com>
CC: Alex Bligh <alex@alex.org.uk>
---
 doc/proto.md | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Eric Blake March 28, 2016, 1:05 p.m. UTC | #1
On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
> From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> 
> It is unclear what the behaviour of a server should be if it receives
> an unknown command. Similar uncertainty exists for command flags.
> 
> Make it explicit that the server should return EINVAL in all such cases.

I'd go one step further and document that for backwards compatibility,
clients should not rely on this particular error.

> 
> Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Wouter Verhelst <w@uter.be>
> CC: Eric Blake <eblake@redhat.com>
> CC: Alex Bligh <alex@alex.org.uk>
> ---
>  doc/proto.md | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index d54ed19..036d6d9 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -512,6 +512,13 @@ return `EINVAL` if it receives a read or trim request including one or
>  more sectors beyond the size of the device.  It also SHOULD map the
>  `EDQUOT` and `EFBIG` errors to `ENOSPC`.  Finally, it SHOULD return
>  `EPERM` if it receives a write or trim request on a read-only export.
> +
> +The server SHOULD return `EINVAL` if it receives an unknown command.
> +
> +The server SHOULD return `EINVAL` if it receives an unknown command flag. It
> +also SHOULD return `EINVAL` if it receives a request with a flag not explicitly
> +documented as applicable to the given request.

In other words, while this is good for the server side, we have proven
that existing server implementations did not follow this, and therefore
it would probably be good to add a sentence along the lines of:

However, clients should not rely on this particular error, as some
historical servers silently ignored invalid commands or flags.

But as what you have proposed is a strict improvement,
Reviewed-by: Eric Blake <eblake@redhat.com>

Patch
diff mbox

diff --git a/doc/proto.md b/doc/proto.md
index d54ed19..036d6d9 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -512,6 +512,13 @@  return `EINVAL` if it receives a read or trim request including one or
 more sectors beyond the size of the device.  It also SHOULD map the
 `EDQUOT` and `EFBIG` errors to `ENOSPC`.  Finally, it SHOULD return
 `EPERM` if it receives a write or trim request on a read-only export.
+
+The server SHOULD return `EINVAL` if it receives an unknown command.
+
+The server SHOULD return `EINVAL` if it receives an unknown command flag. It
+also SHOULD return `EINVAL` if it receives a request with a flag not explicitly
+documented as applicable to the given request.
+
 Which error to return in any other case is not specified by the NBD
 protocol.