[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 09/14] nbd/server: Initial support for extended headers
From: |
Eric Blake |
Subject: |
Re: [PATCH v3 09/14] nbd/server: Initial support for extended headers |
Date: |
Wed, 7 Jun 2023 06:39:08 -0500 |
User-agent: |
NeoMutt/20230517 |
On Wed, May 31, 2023 at 05:46:55PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 15.05.23 22:53, Eric Blake wrote:
> > Time to support clients that request extended headers. Now we can
> > finally reach the code added across several previous patches.
> >
> > Even though the NBD spec has been altered to allow us to accept
> > NBD_CMD_READ larger than the max payload size (provided our response
> > is a hole or broken up over more than one data chunk), we are not
> > planning to take advantage of that, and continue to cap NBD_CMD_READ
> > to 32M regardless of header size.
> >
> > For NBD_CMD_WRITE_ZEROES and NBD_CMD_TRIM, the block layer already
> > supports 64-bit operations without any effort on our part. For
> > NBD_CMD_BLOCK_STATUS, the client's length is a hint, and the previous
> > patch took care of implementing the required
> > NBD_REPLY_TYPE_BLOCK_STATUS_EXT.
> >
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> > nbd/nbd-internal.h | 5 +-
>
> [..]
>
> >
> > static inline void set_be_simple_reply(NBDClient *client, struct iovec
> > *iov,
> > - uint64_t error, NBDRequest *request)
> > + uint32_t error, NBDStructuredError
> > *err,
> > + NBDRequest *request)
> > {
> > - NBDSimpleReply *reply = iov->iov_base;
> > + if (client->header_style >= NBD_HEADER_EXTENDED) {
> > + NBDExtendedReplyChunk *chunk = iov->iov_base;
> >
> > - iov->iov_len = sizeof(*reply);
> > - stl_be_p(&reply->magic, NBD_SIMPLE_REPLY_MAGIC);
> > - stl_be_p(&reply->error, error);
> > - stq_be_p(&reply->handle, request->handle);
> > + iov->iov_len = sizeof(*chunk);
> > + stl_be_p(&chunk->magic, NBD_EXTENDED_REPLY_MAGIC);
> > + stw_be_p(&chunk->flags, NBD_REPLY_FLAG_DONE);
> > + stq_be_p(&chunk->handle, request->handle);
> > + stq_be_p(&chunk->offset, request->from);
> > + if (error) {
> > + assert(!iov[1].iov_base);
> > + iov[1].iov_base = err;
> > + iov[1].iov_len = sizeof(*err);
> > + stw_be_p(&chunk->type, NBD_REPLY_TYPE_ERROR);
> > + stq_be_p(&chunk->length, sizeof(*err));
> > + stl_be_p(&err->error, error);
> > + stw_be_p(&err->message_length, 0);
> > + } else {
> > + stw_be_p(&chunk->type, NBD_REPLY_TYPE_NONE);
> > + stq_be_p(&chunk->length, 0);
> > + }
> > + } else {
> > + NBDSimpleReply *reply = iov->iov_base;
> > +
> > + iov->iov_len = sizeof(*reply);
> > + stl_be_p(&reply->magic, NBD_SIMPLE_REPLY_MAGIC);
> > + stl_be_p(&reply->error, error);
> > + stq_be_p(&reply->handle, request->handle);
> > + }
> > }
> >
> > static int coroutine_fn nbd_co_send_simple_reply(NBDClient *client,
> > @@ -1906,30 +1966,44 @@ static int coroutine_fn
> > nbd_co_send_simple_reply(NBDClient *client,
>
> So, that's not _simple_ now.. The function should be renamed. As well as
> set_be_simple_reply(). _simple_or_extended_ ? a bit too long. But continuing
> to use "simple" is in bad relation with use of "simple" word in specification.
In fact, I added an assertion that set_be_simple_reply() can only be
reached when extended replies are not in use, so none of this
complexity here was needed after all.
>
> Probably better to update callers? The only caller isi
> nbd_send_generic_reply(). So, could we just add
> nbd_co_send_single_extended_reply() to call from nbd_send_generic_reply() in
> case of EXTENDED?
>
> Also, transformation of set_be_simple_reply() do look like it should be two
> separate functions.
>
> > {
> > NBDReply hdr;
> > int nbd_err = system_errno_to_nbd_errno(error);
> > + NBDStructuredError err;
> > struct iovec iov[] = {
> > {.iov_base = &hdr},
> > {.iov_base = data, .iov_len = len}
> > };
> >
> > + assert(!len || !nbd_err);
> > trace_nbd_co_send_simple_reply(request->handle, nbd_err,
> > nbd_err_lookup(nbd_err), len);
> > - set_be_simple_reply(client, &iov[0], nbd_err, request);
> > + set_be_simple_reply(client, &iov[0], nbd_err, &err, request);
> >
> > - return nbd_co_send_iov(client, iov, len ? 2 : 1, errp);
> > + return nbd_co_send_iov(client, iov, iov[1].iov_len ? 2 : 1, errp);
Not introduced in this patch, but it turns out that when
iov[1].iov_len == 0, blindly passing niov==2 to nbd_co_send_iov()
still does the right thing, so I can lose the conditional here.
--
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 09/14] nbd/server: Initial support for extended headers,
Eric Blake <=