qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Dr. David Alan Gilbert
Subject: Re: [Qemu-devel] [PATCH v2 1/1] Make qemu_peek_buffer loop until it gets it's data
Date: Wed, 26 Mar 2014 17:18:49 +0000
User-agent: Mutt/1.5.21 (2010-09-15)

* 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....

> 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.

> > + */
> > +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.

> > @@ -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.

Dave
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK



reply via email to

[Prev in Thread] Current Thread [Next in Thread]