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 <ebl...@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