2:20 p.m.
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 <eblake(a)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