Re: [Qemu-devel] [PATCH v2 1/1] Make qemu_peek_buffer loop until it gets it's data

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <address@hidden> writes:

> From: "Dr. David Alan Gilbert" <address@hidden>
>
> Make qemu_peek_buffer repatedly call fill_buffer until it gets
> all the data it requires, or until there is an error.
>
>   At the moment, qemu_peek_buffer will try one qemu_fill_buffer if there
>   isn't enough data waiting, however the kernel is entitled to return
>   just a few bytes, and still leave qemu_peek_buffer with less bytes
>   than it needed.  I've seen this fail in a dev world, and I think it
>   could theoretically fail in the peeking of the subsection headers in
>   the current world.
>
> Comment qemu_peek_byte to point out it's not guaranteed to work for
>   non-continuous peeks
>
> Use size_t rather than int for size parameters, (and result for
> those functions that never return -errno).

Have you considered doing this cleanup in a separate patch?

Are there any "size or -errno" function values?  If yes, recommend to
make them ssize_t.

> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  include/migration/qemu-file.h | 13 +++++++----
>  qemu-file.c                   | 53 
> +++++++++++++++++++++++++++++++++++--------
>  2 files changed, 52 insertions(+), 14 deletions(-)
>
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index a191fb6..6dd728d 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -121,11 +121,16 @@ static inline void qemu_put_ubyte(QEMUFile *f, unsigned 
> int v)
>  void qemu_put_be16(QEMUFile *f, unsigned int v);
>  void qemu_put_be32(QEMUFile *f, unsigned int v);
>  void qemu_put_be64(QEMUFile *f, uint64_t v);
> -int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset);
> -int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size);
> -int qemu_peek_byte(QEMUFile *f, int offset);
> +size_t qemu_peek_buffer(QEMUFile *f, uint8_t *buf, size_t size, size_t 
> offset);
> +size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size);
> +/*
> + * Note that you can only peek continuous bytes from where the current 
> pointer
> + * is; you aren't guaranteed to be able to peak to +n bytes unless you've
> + * previously peeked +n-1.
> + */
> +int qemu_peek_byte(QEMUFile *f, size_t offset);
>  int qemu_get_byte(QEMUFile *f);
> -void qemu_file_skip(QEMUFile *f, int size);
> +void qemu_file_skip(QEMUFile *f, size_t size);
>  void qemu_update_position(QEMUFile *f, size_t size);
>  
>  static inline unsigned int qemu_get_ubyte(QEMUFile *f)
> diff --git a/qemu-file.c b/qemu-file.c
> index e5ec798..d426136 100644
> --- a/qemu-file.c
> +++ b/qemu-file.c
> @@ -529,7 +529,11 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t 
> block_offset,
>      return RAM_SAVE_CONTROL_NOT_SUPP;
>  }
>  
> -static void qemu_fill_buffer(QEMUFile *f)
> +/*
> + * Attempt to fill the buffer from the underlying file
> + * Returns the number of bytes read, or -ve value for an error.

Please spell out negative.  The clarity gained is well worth five
characters.

Suggest to spell out that this can succeed without filling the buffer
completely, and not just because when hitting EOF.

> + */
> +static int qemu_fill_buffer(QEMUFile *f)

Aha, here's a function value that could become ssize_t.  But then
QEMUFileGetBufferFunc & friends should also be changed.  Feels even more
like a separate patch.

>  {
>      int len;
>      int pending;
> @@ -553,6 +557,8 @@ static void qemu_fill_buffer(QEMUFile *f)
>      } else if (len != -EAGAIN) {
>          qemu_file_set_error(f, len);
>      }
> +
> +    return len;
>  }
>  
>  int qemu_get_fd(QEMUFile *f)
> @@ -676,24 +682,40 @@ void qemu_put_byte(QEMUFile *f, int v)
>      }
>  }
>  
> -void qemu_file_skip(QEMUFile *f, int size)
> +void qemu_file_skip(QEMUFile *f, size_t size)
>  {
>      if (f->buf_index + size <= f->buf_size) {
>          f->buf_index += size;
>      }
>  }
>  
> -int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
> +/*
> + * Read 'size' bytes from file (at 'offset') into buf without moving the
> + * pointer.
> + *
> + * If the underlying fd blocks, then it will return size bytes unless there
> + * was an error, in which case it will return as many as it managed to read.

Begs the question what it'll do when the fd doesn't block.

> + */
> +size_t qemu_peek_buffer(QEMUFile *f, uint8_t *buf, size_t size, size_t 
> offset)
>  {
>      int pending;
>      int index;
>  
>      assert(!qemu_file_is_writable(f));
> +    assert(offset < IO_BUF_SIZE);
> +    assert(size + offset < IO_BUF_SIZE);

Off-by-one?  offset + size <= IO_BUF_SIZE

If you want to guard against overflow: size <= IO_BUF_SIZE - offset.

>  
> +    /* The 1st byte to read from */
>      index = f->buf_index + offset;
> +    /* The number of available bytes starting at index */
>      pending = f->buf_size - index;
> -    if (pending < size) {
> -        qemu_fill_buffer(f);
> +    while (pending < size) {
> +        int received = qemu_fill_buffer(f);
> +
> +        if (received <= 0) {
> +            break;
> +        }
> +
>          index = f->buf_index + offset;
>          pending = f->buf_size - index;
>      }

Loop is useful since qemu_fill_buffer() can succeed without filling the
buffer, and trying again can get it filled.  Correct?

> @@ -709,13 +731,20 @@ int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int 
> size, size_t offset)
>      return size;
>  }
>  
> -int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
> +/*
> + * Read 'size' bytes of data from the file into buf.
> + * 'size' can be larger than the internal buffer.
> + *
> + * If the underlying fd blocks, then it will return size bytes unless there
> + * was an error, in which case it will return as many as it managed to read.

Begs the question what it'll do when the fd doesn't block.

> + */
> +size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
>  {
> -    int pending = size;
> -    int done = 0;
> +    size_t pending = size;
> +    size_t done = 0;
>  
>      while (pending > 0) {
> -        int res;
> +        size_t res;
>  
>          res = qemu_peek_buffer(f, buf, pending, 0);
>          if (res == 0) {
> @@ -729,7 +758,11 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
>      return done;
>  }
>  
> -int qemu_peek_byte(QEMUFile *f, int offset)
> +/*
> + * Peeks a single byte from the buffer; this isn't guaranteed to work if
> + * offset leaves a gap after the previous read/peeked data.
> + */
> +int qemu_peek_byte(QEMUFile *f, size_t offset)
>  {
>      int index = f->buf_index + offset;

assert the offset is sane, like you did in qemu_peek_buffer()?