Re: [Libguestfs] [PATCH v3 10/14] nbd/client: Initial support for extended headers

On 15.05.23 22:53, 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. > > The only caller to nbd_receive_reply() always passed NULL for errp; > since we are changing the signature anyways, I decided to sink the > decision to ignore errors one layer lower. This way nbd_receive_simple_reply() and nbd_receive_structured_reply_chunk() are called now only with explicit NULL last argument.. And we start to drop all errors. Also, actually, we'd better add errp parameter to the caller - nbd_receive_replies(), because its caller (nbd_co_do_receive_one_chunk()) will benefit of it, as it already has errp.

> @@ -1394,28 +1401,34 @@ static int nbd_receive_simple_reply(QIOChannel *ioc, NBDSimpleReply *reply, > > /* nbd_receive_structured_reply_chunk > * Read structured reply chunk except magic field (which should be already > - * read). > + * read). Normalize into the compact form. > * Payload is not read. > */ > -static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, > - NBDStructuredReplyChunk *chunk, > +static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, NBDReply *chunk, > Error **errp) Hmm, _structured_or_extened_ ? Or at least in comment above the function we should mention this.

> { > int ret; > + size_t len; > + uint64_t payload_len; > > - assert(chunk->magic == NBD_STRUCTURED_REPLY_MAGIC); > + if (chunk->magic == NBD_STRUCTURED_REPLY_MAGIC) { > + len = sizeof(chunk->structured); > + } else { > + assert(chunk->magic == NBD_EXTENDED_REPLY_MAGIC); > + len = sizeof(chunk->extended); > + } > > ret = nbd_read(ioc, (uint8_t *)chunk + sizeof(chunk->magic), > - sizeof(*chunk) - sizeof(chunk->magic), "structured chunk", Would be good to print "extended chunk" in error message for EXTENDED case.

> int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, > - NBDReply *reply, Error **errp) > + NBDReply *reply, NBDHeaderStyle hdr) > { > int ret; > const char *type; > > - ret = nbd_read_eof(bs, ioc, &reply->magic, sizeof(reply->magic), errp); > + ret = nbd_read_eof(bs, ioc, &reply->magic, sizeof(reply->magic), NULL); > if (ret <= 0) { > return ret; > } > > reply->magic = be32_to_cpu(reply->magic); > > + /* Diagnose but accept wrong-width header */ > switch (reply->magic) { > case NBD_SIMPLE_REPLY_MAGIC: > - ret = nbd_receive_simple_reply(ioc, &reply->simple, errp); > + if (hdr >= NBD_HEADER_EXTENDED) { > + trace_nbd_receive_wrong_header(reply->magic); maybe, trace also expected style