Re: [PATCH 1/4] Refactor request handling
On Sat, Mar 11, 2023 at 03:07:02PM +0200, w@uter.be wrote:
> From: Wouter Verhelst <w@uter.be>
>
> Currently, the state of a request is stored on the stack, with the reply
> being built in a buffer which is sent out at the very end of the
> 'handle_normal_read' function.
>
> This makes implementing structured replies complicated, as for those we
> may want to sometimes send a reply chunk before that point.
>
> Thus, refactor request handling such that we no longer depend on the
> stack for sending out things.
>
> Signed-off-by: Wouter Verhelst <w@uter.be>
> ---
> nbd-server.c | 144 +++++++++++++++++++++++++++++++++++----------------
> nbdsrv.h | 8 +++
> 2 files changed, 106 insertions(+), 46 deletions(-)
> /**
> * Read an amount of bytes at a given offset from the right file. This
> * abstracts the read-side of the multiple files option.
> @@ -1310,15 +1354,20 @@ ssize_t rawexpread(off_t a, char *buf, size_t len, CLIENT *client) {
> * Call rawexpread repeatedly until all data has been read.
> * @return 0 on success, nonzero on failure
> **/
> -int rawexpread_fully(off_t a, char *buf, size_t len, CLIENT *client) {
> +int rawexpread_fully(READ_CTX *ctx, CLIENT *client) {
> ssize_t ret=0;
>
> - while(len > 0 && (ret=rawexpread(a, buf, len, client)) > 0 ) {
> - a += ret;
> - buf += ret;
> - len -= ret;
> + char *buf;
> +
> + while(ctx->current_len > 0) {
> + buf = find_read_buf(ctx);
> + if((ret = rawexpread((off_t)ctx->req->from + (off_t)ctx->current_offset, buf, ctx->current_len, client)) < 0) {
> + return ret;
> + }
> + ctx->current_offset += ret;
> + ctx->current_len -= ret;
> }
> - return (ret < 0 || len != 0);
The old code looped until an error was detected or a short read occurs
(ret == 0, len != 0).
> + return (ret < 0 || ctx->current_len != 0);
But the new code loops only until ctx->current_len is 0 (with early
exit on read error), making the right side of the || dead code, and
making the loop itself busy-loop on a persistently short read.
Quickest fix would be inserting
if (ret == 0) break;
after the if((ret = ...)) condition, and leaving the || in the return
untouched.
...
> +++ b/nbdsrv.h
> @@ -89,6 +89,14 @@ typedef struct {
> off_t startoff; /**< starting offset of this file */
> } FILE_INFO;
>
> +typedef struct {
> + struct nbd_request *req;
> + char *buf;
> + size_t buflen;
> + size_t current_offset;
Should this be an off_t to begin with, or do we not care about 32-bit platforms?
> + uint32_t current_len;
> +} READ_CTX;
> +
> /* Constants and macros */
>
> /**
> --
> 2.39.2
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Reply to: