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

[Markus Armbruster]

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

> * Markus Armbruster (address@hidden) wrote:
>> "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?
>
> I'd just taken the ones involved in the functions I was
> changing anyway, and it seemed small enough to roll in, but yes I can
> do that.
>
>> Are there any "size or -errno" function values?  If yes, recommend to
>> make them ssize_t.
>
> Yes there are a few.
>
>> 
>> > 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.
>
> I can see I'm going to need a mod to checkpatch for this....

Heh.  Retraining fingers can be hard :)

>> Suggest to spell out that this can succeed without filling the buffer
>> completely, and not just because when hitting EOF.
>
> Yep I can clarify that.
>
>> > + */
>> > +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.
>
> Yes it could; I'd not considered that but it does make more sense than
> int.   However, changing that will mean I also need to change more other
> places, so yeh, 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.
>
> At the moment the migration stream stuff is always set up to block (although
> in the postcopy world that's changed and this becomes more 'interesting'), 
> still
> if it begs that question then at the moment it's undefined, and I don't see
> a reason to define it until we use it that way/figure out what the
> best thing is.

Yes, specifying behavior without use cases doesn't sound useful.

Easy fix for the comment: turn the else-less conditional "if the
underlying fd blocks" into an explicit design assumption.

>> > + */
>> > +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
>
> Hmm good catch.
>
>> 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?
>
> Correct; I'll add a comment to make it explicit.

With qemu_fill_buffer()'s contract clarified as discussed above, a
comment might not be necessary.  Your choice.

>> > @@ -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.
>
> As above.
>
>> > +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()?
>
> Yep, can do.

Thanks!