The subject lines are confusing: 9/14 enables extended headers in the server, while this one does not yet enable the headers but is merely a preliminary step. I'll probably retitle one or the other in v4.
On Wed, May 31, 2023 at 06:26:17PM +0300, Vladimir Sementsov-Ogievskiy wrote: > 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. I can explore plumbing errp back through for v4. > > @@ -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. I'm going with 'nbd_receive_reply_chunk', since both structured and extended modes receive chunks. > > > { > > 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. Or even just "chunk header", which covers both modes. > > 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 Sure, I can give that a shot. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org