Re: [Qemu-devel] [PATCH 2/2] migration/qemu-file: fix potential buf waste for extra buf_index adjustment

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Wei Yang (richard.weiyang@gmail.com) wrote:
> On Fri, Aug 23, 2019 at 12:06:09PM +0100, Dr. David Alan Gilbert wrote:
> >(Copying Dan in)
> >
> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
> >> In add_to_iovec(), qemu_fflush() will be called if iovec is full. If
> >> this happens, buf_index is reset. Currently, this is not checked and
> >> buf_index would always been adjust with buf size.
> >> 
> >> This is not harmful, but will waste some space in file buffer.
> >
> >That's a nice find.
> >
> >> This patch make add_to_iovec() return 1 when it has flushed the file.
> >> Then the caller could check the return value to see whether it is
> >> necessary to adjust the buf_index any more.
> >> 
> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >
> >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> >(I wonder if there's a way to wrap that little add_to_iovec, check, add
> >to index, flush in a little wrapper).
> >
> >Dave
> >
> >> ---
> >>  migration/qemu-file.c | 42 ++++++++++++++++++++++++++++--------------
> >>  1 file changed, 28 insertions(+), 14 deletions(-)
> >> 
> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> >> index 35c22605dd..05d9f42ddb 100644
> >> --- a/migration/qemu-file.c
> >> +++ b/migration/qemu-file.c
> >> @@ -343,8 +343,16 @@ int qemu_fclose(QEMUFile *f)
> >>      return ret;
> >>  }
> >>  
> >> -static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size,
> >> -                         bool may_free)
> >> +/*
> >> + * Add buf to iovec. Do flush if iovec is full.
> >> + *
> >> + * Return values:
> >> + * 1 iovec is full and flushed
> >> + * 0 iovec is not flushed
> >> + *
> >> + */
> >> +static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size,
> >> +                        bool may_free)
> >>  {
> >>      /* check for adjacent buffer and coalesce them */
> >>      if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base +
> >> @@ -362,7 +370,10 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size,
> >>  
> >>      if (f->iovcnt >= MAX_IOV_SIZE) {
> >>          qemu_fflush(f);
> >> +        return 1;
> >>      }
> >> +
> >> +    return 0;
> >>  }
> >>  
> >>  void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
> >> @@ -391,10 +402,11 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size)
> >>          }
> >>          memcpy(f->buf + f->buf_index, buf, l);
> >>          f->bytes_xfer += l;
> >> -        add_to_iovec(f, f->buf + f->buf_index, l, false);
> >> -        f->buf_index += l;
> >> -        if (f->buf_index == IO_BUF_SIZE) {
> >> -            qemu_fflush(f);
> >> +        if (!add_to_iovec(f, f->buf + f->buf_index, l, false)) {
> >> +            f->buf_index += l;
> >> +            if (f->buf_index == IO_BUF_SIZE) {
> >> +                qemu_fflush(f);
> >> +            }
> 
> You mean put these four lines into a wrapper?
> 
> Name it as add_buf_to_iovec?

Yes.

Dave

> >>          }
> >>          if (qemu_file_get_error(f)) {
> >>              break;
> >> @@ -412,10 +424,11 @@ void qemu_put_byte(QEMUFile *f, int v)
> >>  
> >>      f->buf[f->buf_index] = v;
> >>      f->bytes_xfer++;
> >> -    add_to_iovec(f, f->buf + f->buf_index, 1, false);
> >> -    f->buf_index++;
> >> -    if (f->buf_index == IO_BUF_SIZE) {
> >> -        qemu_fflush(f);
> >> +    if (!add_to_iovec(f, f->buf + f->buf_index, 1, false)) {
> >> +        f->buf_index++;
> >> +        if (f->buf_index == IO_BUF_SIZE) {
> >> +            qemu_fflush(f);
> >> +        }
> >>      }
> >>  }
> >>  
> >> @@ -717,10 +730,11 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream,
> >>      }
> >>  
> >>      qemu_put_be32(f, blen);
> >> -    add_to_iovec(f, f->buf + f->buf_index, blen, false);
> >> -    f->buf_index += blen;
> >> -    if (f->buf_index == IO_BUF_SIZE) {
> >> -        qemu_fflush(f);
> >> +    if (!add_to_iovec(f, f->buf + f->buf_index, blen, false)) {
> >> +        f->buf_index += blen;
> >> +        if (f->buf_index == IO_BUF_SIZE) {
> >> +            qemu_fflush(f);
> >> +        }
> >>      }
> >>      return blen + sizeof(int32_t);
> >>  }
> >> -- 
> >> 2.17.1
> >> 
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK