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 <stefa...@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
signature.asc
Description: PGP signature
