On Tue, Jun 27, 2023 at 05:22:09PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.06.23 16:56, Eric Blake wrote:
> > Update the client code to be able to send an extended request, and
> > parse an extended header from the server. Note that since we reject
> > any structured reply with a too-large payload, we can always normalize
> > a valid header back into the compact form, so that the caller need not
> > deal with two branches of a union. Still, until a later patch lets
> > the client negotiate extended headers, the code added here should not
> > be reached. Note that because of the different magic numbers, it is
> > just as easy to trace and then tolerate a non-compliant server sending
> > the wrong header reply as it would be to insist that the server is
> > compliant.
> >
> > Signed-off-by: Eric Blake <ebl...@redhat.com>
> > ---
> > @@ -1508,23 +1537,28 @@ int coroutine_fn nbd_receive_reply(BlockDriverState
> > *bs, QIOChannel *ioc,
> > reply->cookie);
case NBD_SIMPLE_REPLY_MAGIC:
if (mode >= NBD_MODE_EXTENDED) {
trace_nbd_receive_wrong_header(reply->magic,
nbd_mode_lookup(mode));
}
ret = nbd_receive_simple_reply(ioc, &reply->simple, errp);
if (ret < 0) {
break;
}
trace_nbd_receive_simple_reply(reply->simple.error,
nbd_err_lookup(reply->simple.error),
reply->cookie);
> > break;
> > case NBD_STRUCTURED_REPLY_MAGIC:
> > - ret = nbd_receive_structured_reply_chunk(ioc, &reply->structured,
> > errp);
> > + case NBD_EXTENDED_REPLY_MAGIC:
> > + expected = mode >= NBD_MODE_EXTENDED ? NBD_EXTENDED_REPLY_MAGIC
> > + : NBD_STRUCTURED_REPLY_MAGIC;
> > + if (reply->magic != expected) {
> > + trace_nbd_receive_wrong_header(reply->magic,
> > + nbd_mode_lookup(mode));
> > + }
> > + ret = nbd_receive_reply_chunk_header(ioc, reply, errp);
> > if (ret < 0) {
> > break;
> > }
>
> maybe
>
> assert(reply->magic == NBD_STRUCTURED_REPLY_MAGIC)
No, not even as a temporary assertion. You are correct that as of
this patch, a compliant server will only be sending structured
replies, not extended (because we haven't turned on a request for
extended headers yet); but we should never assert something that a
non-compliant server can send over the wire. So the best we can do is
trace the server's unusual response.
>
> > type = nbd_reply_type_lookup(reply->structured.type);
> > - trace_nbd_receive_structured_reply_chunk(reply->structured.flags,
> > - reply->structured.type,
> > type,
> > - reply->structured.cookie,
> > - reply->structured.length);
> > + trace_nbd_receive_reply_chunk_header(reply->structured.flags,
> > + reply->structured.type, type,
> > + reply->structured.cookie,
> > + reply->structured.length);
> > break;
> > default:
> > + trace_nbd_receive_wrong_header(reply->magic,
> > nbd_mode_lookup(mode));
> > error_setg(errp, "invalid magic (got 0x%" PRIx32 ")",
> > reply->magic);
> > return -EINVAL;
> > }
> > - if (ret < 0) {
> > - return ret;
> > - }
>
> hmm, mistake? Seems, we'll return 1 on error with set errp this way
>
> >
> > return 1;
Indeed, I botched that logic (either keep the 'ret < 0' filter after
the switch, or change 'break' to 'return ret' within the switch).
Will fix in v5.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
