[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [PATCH 1/4] Refactor request handling

On Mon, Mar 27, 2023 at 12:33:06PM +0200, Wouter Verhelst wrote:
> > ....
> > > +++ 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?

Until we have 64-bit extensions implemented (yes, I know I need to
post v3 of my work on that series), it doesn't matter.  And even with
64-bit extensions, you are right that read and write commands bearing
a payload will still tend to be limited by ssize_t or smaller (even
size_t may be too big, depending on which underlying kernel syscalls
we are utilizing), where we aren't likely to overflow an offset in a
structured reply if we never allow a read request >4G in the first
place.  So on re-reading my question and your response, I think you
are okay with size_t for current_offset.

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Reply to: