[Libguestfs] [libnbd PATCH 4/4] states: Use RESYNC to handle more structured reply server bugs

[Eric Blake]

On Thu, Aug 11, 2022 at 09:06:38PM -0500, Eric Blake wrote:
> 
> Again, compliant servers will never trip over to the new state; but an
> easy way to demonstrate this in action is with a temporary patch to
> nbdkit:

That tested one of the error paths; I found a few others where the
patch needed tweaks:

> @@ -222,13 +181,25 @@ STATE_MACHINE {
>      return 0;
>    case 0:
>      length = be32toh (h->sbuf.sr.structured_reply.length);
> +    assert (length >= sizeof h->sbuf.sr.payload.error.error.error);
> +    assert (cmd);
> +
> +    if (length < sizeof h->sbuf.sr.payload.error.error) {
> +    resync:
> +      /* Favor the error packet's errno over RESYNC's EPROTO. */
> +      error = be32toh (h->sbuf.sr.payload.error.error.error);
> +      if (cmd->error = 0)

Typo: that should be == rather than =.

> +        cmd->error = nbd_internal_errno_of_nbd_error (error);
> +      h->rbuf = NULL;
> +      h->rlen = length - MIN (length, sizeof h->sbuf.sr.payload.error.error);
> +      SET_NEXT_STATE (%RESYNC);
> +      return 0;
> +    }
> +
>      msglen = be16toh (h->sbuf.sr.payload.error.error.len);
...

> @@ -257,24 +228,21 @@ STATE_MACHINE {
>        debug (h, "structured error server message: %.*s", (int) msglen,
>               h->sbuf.sr.payload.error.msg);
> 
> -    /* Special case two specific errors; ignore the tail for all others */
> +    /* Special case two specific errors; silently ignore tail for all others */
>      h->rbuf = NULL;
>      h->rlen = length;
>      switch (type) {
>      case NBD_REPLY_TYPE_ERROR:
> -      if (length != 0) {
> -        SET_NEXT_STATE (%.DEAD);
> -        set_error (0, "error payload length too large");
> -        return 0;
> -      }
> +      if (length != 0)
> +        debug (h, "ignoring unexpected slop after error message, "
> +               "the server may have a bug");
>        break;
>      case NBD_REPLY_TYPE_ERROR_OFFSET:
> -      if (length != sizeof h->sbuf.sr.payload.error.offset) {
> -        SET_NEXT_STATE (%.DEAD);
> -        set_error (0, "invalid error payload length");
> -        return 0;
> -      }
> -      h->rbuf = &h->sbuf.sr.payload.error.offset;
> +      if (length == sizeof h->sbuf.sr.payload.error.offset)

and that should be !=, not ==.

> +        debug (h, "unable to safely extract error offset, "
> +               "the server may have a bug");
> +      else
> +        h->rbuf = &h->sbuf.sr.payload.error.offset;
>        break;
>      }
>      SET_NEXT_STATE (%RECV_ERROR_TAIL);

Plus a typo in the subject for 1/4.  With everything fixed, the series
is now in as 185195d..0883029.

Another leniency issue I am exploring is whether structured reply mode
can cope with a server that mistakenly sends a simply reply to
NBD_CMD_READ, and whether the client can ignore replies for an unknown
cookie.

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