Re: [PATCH 1/4] Refactor request handling

[Wouter Verhelst <w@uter.be>]

On Tue, Mar 14, 2023 at 10:44:54AM -0500, Eric Blake wrote:
> 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.

Thanks, good catch.

I've changed it to 

if((ret = rawexpread(...)) <= 0) {
	break;
}

which seems like an even better idea (and is closer to the original
behavior).

> ....
> > +++ 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?

current_offset holds how much of the request we have already handled,
not a file offset. In the case of simple replies (or structured replies
with DF set), the reply is built into an allocated buffer, and in that
case current_offset is used in pointer arithmetic (which implies it
needs to be a size_t, or be at least cast to that).

It's true that it isn't used in pointer arithmetics in the case of
structured replies, since there we allocate a fresh buffer every time we
get into find_read_buf. However, I still want to think of file_read_buf
as a function returning a, possibly offset, pointer into a buffer even
in that case, which also implies it should be a size_t rather than an
off_t.

Of course, in either case current_offset is *added* to a file offset in
order to find the point where we should start reading, but that is only
a secondary usage, not a primary one.

But perhaps I'm missing something?

-- 
     w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}

I will have a Tin-Actinium-Potassium mixture, thanks.