Re: [PATCH 3/4] qemu-io: add -r option to register I/O buffer

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3093 bytes --]

On Fri, Jan 27, 2023 at 05:24:30PM -0600, Eric Blake wrote:
> On Thu, Jan 26, 2023 at 03:14:00PM -0500, Stefan Hajnoczi wrote:
> > The blk_register_buf() API is an optimization hint that allows some
> > block drivers to avoid I/O buffer housekeeping or bounce buffers.
> > 
> > Add an -r option to register the I/O buffer so that qemu-io can be used
> > to test the blk_register_buf() API. The next commit will add a test that
> > uses the new option.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  qemu-io-cmds.c | 149 ++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 103 insertions(+), 46 deletions(-)
> > 
> 
> >  static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> > -                                     const char *file_name)
> > +                                     const char *file_name, bool register_buf)
> >  {
> >      char *buf, *buf_origin;
> >      FILE *f = fopen(file_name, "r");
> > @@ -414,6 +423,10 @@ static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> >      fclose(f);
> >      f = NULL;
> >  
> > +    if (register_buf) {
> > +        blk_register_buf(blk, buf_origin, len, &error_abort);
> > +    }
> 
> Why is this conditionally true...
> 
> > +
> >      if (len > pattern_len) {
> >          len -= pattern_len;
> >          buf += pattern_len;
> > @@ -431,7 +444,7 @@ static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> >      return buf_origin;
> >  
> >  error:
> > -    qemu_io_free(buf_origin);
> > +    qemu_io_free(blk, buf_origin, len, false);
> 
> ...but cleanup unconditionally false?

It looks suspicious but it's correct: goto error only happens before the
blk_register_buf() call. Therefore we must *not* unregister the buffer
at this point.

I'll add a comment to make this clear.

> 
> > @@ -719,6 +732,7 @@ static void read_help(void)
> >  " -p, -- ignored for backwards compatibility\n"
> >  " -P, -- use a pattern to verify read data\n"
> >  " -q, -- quiet mode, do not show I/O statistics\n"
> > +" -r, -- register I/O buffer\n"
> >  " -s, -- start offset for pattern verification (only with -P)\n"
> >  " -v, -- dump buffer to standard output\n"
> 
> Here we have kept more or less alphabetical...
> 
> > @@ -909,6 +932,7 @@ static void readv_help(void)
> >  " -P, -- use a pattern to verify read data\n"
> >  " -v, -- dump buffer to standard output\n"
> >  " -q, -- quiet mode, do not show I/O statistics\n"
> > +" -r, -- register I/O buffer\n"
> 
> ...here we have not.  While it's pre-existing, would it be worth
> touching up while you're here?
> 
> > @@ -1455,6 +1503,7 @@ static void aio_read_help(void)
> >  " -i, -- treat request as invalid, for exercising stats\n"
> >  " -v, -- dump buffer to standard output\n"
> >  " -q, -- quiet mode, do not show I/O statistics\n"
> > +" -r, -- register I/O buffer\n"
> 
> Another pre-existing spot.
> 
> Overall makes sense to me.

Sure, I'll order the help output alphabetically in the next revision.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]