[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 10/14] nbd/client: Initial support for extended headers
From: |
Eric Blake |
Subject: |
Re: [PATCH v3 10/14] nbd/client: Initial support for extended headers |
Date: |
Wed, 7 Jun 2023 13:22:27 -0500 |
User-agent: |
NeoMutt/20230517 |
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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v3 10/14] nbd/client: Initial support for extended headers,
Eric Blake <=