Libguestfs - May 2019 - Re: [nbdkit PATCH 3/7] RFC: protocol: Only send EOVERFLOW when valid

On Mon, Apr 22, 2019 at 07:50:22PM -0500, Eric Blake
wrote:
> Previously, we were squashing EOVERFLOW into EINVAL; continue to do so
> at points in the protocol where the client may not be expecting
> EOVERFLOW.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  server/protocol.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/server/protocol.c b/server/protocol.c
> index a52bb56..0a9f73c 100644
> --- a/server/protocol.c
> +++ b/server/protocol.c
> @@ -326,7 +326,7 @@ skip_over_write_buffer (int sock, size_t count)
> 
>  /* Convert a system errno to an NBD_E* error code. */
>  static int
> -nbd_errno (int error)
> +nbd_errno (int error, bool flag_df)
>  {
>    switch (error) {
>    case 0:
> @@ -349,7 +349,9 @@ nbd_errno (int error)
>      return NBD_ESHUTDOWN;
>  #endif
>    case EOVERFLOW:
> -    return NBD_EOVERFLOW;
> +    if (flag_df)
> +      return NBD_EOVERFLOW;
> +    /* fallthrough */
>    case EINVAL:
>    default:
>      return NBD_EINVAL;
> @@ -368,7 +370,7 @@ send_simple_reply (struct connection *conn,
> 
>    reply.magic = htobe32 (NBD_SIMPLE_REPLY_MAGIC);
>    reply.handle = handle;
> -  reply.error = htobe32 (nbd_errno (error));
> +  reply.error = htobe32 (nbd_errno (error, false));
> 
>    r = conn->send (conn, &reply, sizeof reply);
>    if (r == -1) {
> @@ -573,7 +575,8 @@ send_structured_reply_block_status (struct connection
*conn,
> 
>  static int
>  send_structured_reply_error (struct connection *conn,
> -                             uint64_t handle, uint16_t cmd, uint32_t
error)
> +                             uint64_t handle, uint16_t cmd, uint16_t
flags,
> +                             uint32_t error)
>  {
>    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock);
>    struct structured_reply reply;
> @@ -593,7 +596,7 @@ send_structured_reply_error (struct connection *conn,
>    }
> 
>    /* Send the error. */
> -  error_data.error = htobe32 (nbd_errno (error));
> +  error_data.error = htobe32 (nbd_errno (error, flags &
NBD_CMD_FLAG_DF));
>    error_data.len = htobe16 (0);
>    r = conn->send (conn, &error_data, sizeof error_data);
>    if (r == -1) {
> @@ -737,7 +740,8 @@ protocol_recv_request_send_reply (struct connection
*conn)
>                                                     extents);
>      }
>      else
> -      return send_structured_reply_error (conn, request.handle, cmd,
error);
> +      return send_structured_reply_error (conn, request.handle, cmd,
flags,
> +                                          error);
>    }
>    else
>      return send_simple_reply (conn, request.handle, cmd, buf, count,
error);
> -- 
> 2.20.1
The protocol spec is unclear on whether EOVERFLOW can be returned in
cases other than the DF flag being set.  Whether we include this patch
or not seems to hinge on the interpretation of the protocol spec which
I'm not really in a position to make.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

[adding NBD list]

On 4/23/19 2:36 AM, Richard W.M. Jones wrote:
> On Mon, Apr 22, 2019 at 07:50:22PM -0500, Eric Blake wrote:
>> Previously, we were squashing EOVERFLOW into EINVAL; continue to do so
>> at points in the protocol where the client may not be expecting
>> EOVERFLOW.
> 
> The protocol spec is unclear on whether EOVERFLOW can be returned in
> cases other than the DF flag being set.  Whether we include this patch
> or not seems to hinge on the interpretation of the protocol spec which
> I'm not really in a position to make.
Context: nbdkit previously did not allow the EOVERFLOW value over the
wire, so I'm proposing a patch to nbdkit to support it.  But it raises
the question on whether allowing EOVERFLOW to any arbitrary command is
okay, or whether EOVERFLOW should only be exposed over the wire to a
client that is likely to expect it. The NBD spec added EOVERFLOW as a
valid error value when commit 7ff2e45e (Apr 2016) promoted structured
replies to be part of the protocol, so any client that negotiates
structured replies is thus new enough to expect EOVERFLOW; conversely,
EOVERFLOW is only documented as being reasonable for the server to send
in response to a client using NBD_CMD_FLAG_DF to NBD_CMD_READ (again,
which implies the client negotiated structured replies), and not all
clients support structured replies.

See also my recent thread about the potential of adding ENOTSUP as a
valid error, towards the end of the message:
https://lists.debian.org/nbd/2019/03/msg00007.html

There, I argued that since clients do not have to have a 1:1 mapping
from system errno values to NBD wire error values, that any addition of
a new error value over the wire should at least think about back-compat
considerations, where we should consider documenting that a server
SHOULD NOT send the new error except to clients that are obviously new
enough to be aware that the error is possible.  I intentionally used
SHOULD NOT rather than MUST NOT, as a server may still choose to expose
non-standard errors over the wire if a client might benefit from those
errors, and a well-written client will squash non-standard errors
received over the wire back to EINVAL.

So the question at hand is whether I should patch the NBD spec to
recommend that a server SHOULD NOT send EOVERFLOW except in response to
NBD_CMD_READ when the NBD_CMD_FLAG_DF bit is set (similar to my proposed
wording that a server SHOULD NOT send ENOTSUP except in response to
NBD_CMD_FLAG_FAST_ZERO).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 4/23/19 7:38 AM, Eric Blake wrote:
> [adding NBD list]
> 
> On 4/23/19 2:36 AM, Richard W.M. Jones wrote:
>> On Mon, Apr 22, 2019 at 07:50:22PM -0500, Eric Blake wrote:
>>> Previously, we were squashing EOVERFLOW into EINVAL; continue to do
so
>>> at points in the protocol where the client may not be expecting
>>> EOVERFLOW.
> 
>>
>> The protocol spec is unclear on whether EOVERFLOW can be returned in
>> cases other than the DF flag being set.  Whether we include this patch
>> or not seems to hinge on the interpretation of the protocol spec which
>> I'm not really in a position to make.
...
> 
> So the question at hand is whether I should patch the NBD spec to
> recommend that a server SHOULD NOT send EOVERFLOW except in response to
> NBD_CMD_READ when the NBD_CMD_FLAG_DF bit is set (similar to my proposed
> wording that a server SHOULD NOT send ENOTSUP except in response to
> NBD_CMD_FLAG_FAST_ZERO).
As I have now pushed my proposed changes to the NBD spec to clarify
this, I am also pushing the nbdkit patch to handle EOVERFLOW only when
NBD_CMD_FLAG_DF is in use.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Apr 23, 2019 at 07:38:41AM -0500, Eric Blake
wrote:
> [adding NBD list]
> 
> On 4/23/19 2:36 AM, Richard W.M. Jones wrote:
> > On Mon, Apr 22, 2019 at 07:50:22PM -0500, Eric Blake wrote:
> >> Previously, we were squashing EOVERFLOW into EINVAL; continue to
do so
> >> at points in the protocol where the client may not be expecting
> >> EOVERFLOW.
> 
> > 
> > The protocol spec is unclear on whether EOVERFLOW can be returned in
> > cases other than the DF flag being set.  Whether we include this patch
> > or not seems to hinge on the interpretation of the protocol spec which
> > I'm not really in a position to make.
> 
> Context: nbdkit previously did not allow the EOVERFLOW value over the
> wire, so I'm proposing a patch to nbdkit to support it.  But it raises
> the question on whether allowing EOVERFLOW to any arbitrary command is
> okay, or whether EOVERFLOW should only be exposed over the wire to a
> client that is likely to expect it. The NBD spec added EOVERFLOW as a
> valid error value when commit 7ff2e45e (Apr 2016) promoted structured
> replies to be part of the protocol, so any client that negotiates
> structured replies is thus new enough to expect EOVERFLOW; conversely,
> EOVERFLOW is only documented as being reasonable for the server to send
> in response to a client using NBD_CMD_FLAG_DF to NBD_CMD_READ (again,
> which implies the client negotiated structured replies), and not all
> clients support structured replies.
OTOH, for backcompat reasons it is reasonable to state that older
versions of nbd-server could send pretty much anything over the wire[1],
and that clients should therefore treat any nonzero value as an unknown
error.

I think that might also be a correct way to deal with error numbers in
cases where the client does not know what to do with it.

[1] I originally thought that errno values were way more standardized
than they happen to be in practice, and so the initial error handling in
nbd-server just sent the value of errno, whatever it happened to be,
over the wire. That worked just fine if client and server were the same
platform -- and more so since all the client would ever do when it saw
an error was yell "the server sent error code X" and abort, so that
the
actual error number didn't even matter -- but it obviously wasn't ideal;
and when we chose the error values that got written in stone, we chose
the errno values that Linux/x86 uses for the types of errors that seemed
reasonable. What older servers sent is however not really defined, and
therefore should be treated as nasal daemons.

[...]
> SHOULD NOT rather than MUST NOT, as a server may still choose to expose
> non-standard errors over the wire if a client might benefit from those
> errors, and a well-written client will squash non-standard errors
> received over the wire back to EINVAL.
Indeed; I think that is what we should do.
> So the question at hand is whether I should patch the NBD spec to
> recommend that a server SHOULD NOT send EOVERFLOW except in response to
> NBD_CMD_READ when the NBD_CMD_FLAG_DF bit is set (similar to my proposed
> wording that a server SHOULD NOT send ENOTSUP except in response to
> NBD_CMD_FLAG_FAST_ZERO).
I think we can say that, but we should definitely also say that a client
should treat unknown errors in a particular way. Possibly "abort the
connection and give up", but something.

-- 
To the thief who stole my anti-depressants: I hope you're happy

  -- seen somewhere on the Internet on a photo of a billboard