On Fri, Jun 16, 2023 at 09:48:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.06.23 16:56, Eric Blake wrote:
> > Although extended mode is not yet enabled, once we do turn it on, we
> > need to reply with extended headers to all messages. Update the low
> > level entry points necessary so that all other callers automatically
> > get the right header based on the current mode.
> >
> > Signed-off-by: Eric Blake <ebl...@redhat.com>
> > ---
> >
> > v4: new patch, split out from v3 9/14
> > ---
> > nbd/server.c | 30 ++++++++++++++++++++++--------
> > 1 file changed, 22 insertions(+), 8 deletions(-)
> >
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 119ac765f09..84c848a31d3 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -1947,8 +1947,6 @@ static inline void set_be_chunk(NBDClient *client,
> > struct iovec *iov,
> > size_t niov, uint16_t flags, uint16_t
> > type,
> > NBDRequest *request)
> > {
> > - /* TODO - handle structured vs. extended replies */
> > - NBDStructuredReplyChunk *chunk = iov->iov_base;
> > size_t i, length = 0;
> >
> > for (i = 1; i < niov; i++) {
> > @@ -1956,12 +1954,26 @@ static inline void set_be_chunk(NBDClient *client,
> > struct iovec *iov,
> > }
> > assert(length <= NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData));
> >
> > - iov[0].iov_len = sizeof(*chunk);
> > - stl_be_p(&chunk->magic, NBD_STRUCTURED_REPLY_MAGIC);
> > - stw_be_p(&chunk->flags, flags);
> > - stw_be_p(&chunk->type, type);
> > - stq_be_p(&chunk->cookie, request->cookie);
> > - stl_be_p(&chunk->length, length);
> > + if (client->mode >= NBD_MODE_EXTENDED) {
> > + NBDExtendedReplyChunk *chunk = iov->iov_base;
> > +
> > + iov->iov_len = sizeof(*chunk);
>
> I'd prefer to keep iov[0].iov_len notation, to stress that iov is an array
>
> anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
I can make that change, and keep your R-b.
>
> > + stl_be_p(&chunk->magic, NBD_EXTENDED_REPLY_MAGIC);
> > + stw_be_p(&chunk->flags, flags);
> > + stw_be_p(&chunk->type, type);
> > + stq_be_p(&chunk->cookie, request->cookie);
>
> Hm. Not about this patch:
>
> we now moved to simple cookies. And it seems that actually, 64bit is too much
> for number of request.
You're right that it's more than qemu cared about. But there may be
other implementations that really do store a 64-bit pointer as their
opaque cookie, for ease of reverse-lookup on which command the
server's reply corresponds to, so I don't see it changing any time
soon in the NBD protocol.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
