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

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

* ???? (chenliang0016@icloud.com) wrote:
> 
> ?? 2014??4??8????????10:29??Dr. David Alan Gilbert (git) <dgilbert@redhat.com> ??????
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Make qemu_peek_buffer repeatedly 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.
> hmm, I also have got some errors(infrequently). Maybe It is one point.
> Could you show some messages about the error?

I've only seen the errors in my visitor/ber world where I use the peek_buffer
a lot more; but the one place it is used in the existing code is in
the code to check if we have the start of a subsection; if that goes wrong
I'm not sure what error would be produced.

Dave

> 
> However, this patch avoids one potential issue, thanks.
> 
> Reviewed-by: ChenLiang <chenliang0016@icloud.com>
> > 
> > Comment qemu_peek_byte to point out it's not guaranteed to work for
> >  non-continuous peeks
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> > 
> > v4:
> >  Limit the size qemu_get_buffer passes to qemu_fill_buffer on each loop
> > 
> > v3:
> >  Make qemu_fill_buffer return ssize_t
> >  Remove the other size_t cleanup from this patch - I'll get back to them later
> >  Comment additions/cleanups from review
> >  Stretch my -ve
> > 
> > include/migration/qemu-file.h |  5 ++++
> > qemu-file.c                   | 53 +++++++++++++++++++++++++++++++++++++++----
> > 2 files changed, 54 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> > index a191fb6..c90f529 100644
> > --- a/include/migration/qemu-file.h
> > +++ b/include/migration/qemu-file.h
> > @@ -123,6 +123,11 @@ 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);
> > +/*
> > + * 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, int offset);
> > int qemu_get_byte(QEMUFile *f);
> > void qemu_file_skip(QEMUFile *f, int size);
> > diff --git a/qemu-file.c b/qemu-file.c
> > index e5ec798..6ae37d0 100644
> > --- a/qemu-file.c
> > +++ b/qemu-file.c
> > @@ -529,7 +529,15 @@ 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 negative value for an error.
> > + *
> > + * Note that it can return a partially full buffer even in a not error/not EOF
> > + * case if the underlying file descriptor gives a short read, and that can
> > + * happen even on a blocking fd.
> > + */
> > +static ssize_t qemu_fill_buffer(QEMUFile *f)
> > {
> >     int len;
> >     int pending;
> > @@ -553,6 +561,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)
> > @@ -683,17 +693,39 @@ void qemu_file_skip(QEMUFile *f, int size)
> >     }
> > }
> > 
> > +/*
> > + * Read 'size' bytes from file (at 'offset') into buf without moving the
> > + * pointer.
> > + *
> > + * It will return size bytes unless there was an error, in which case it will
> > + * return as many as it managed to read (assuming blocking fd's which
> > + * all current QEMUFile are)
> > + */
> > int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
> > {
> >     int pending;
> >     int index;
> > 
> >     assert(!qemu_file_is_writable(f));
> > +    assert(offset < IO_BUF_SIZE);
> > +    assert(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);
> > +
> > +    /*
> > +     * qemu_fill_buffer might return just a few bytes, even when there isn't
> > +     * an error, so loop collecting them until we get enough.
> > +     */
> > +    while (pending < size) {
> > +        int received = qemu_fill_buffer(f);
> > +
> > +        if (received <= 0) {
> > +            break;
> > +        }
> > +
> >         index = f->buf_index + offset;
> >         pending = f->buf_size - index;
> >     }
> > @@ -709,6 +741,14 @@ int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
> >     return size;
> > }
> > 
> > +/*
> > + * Read 'size' bytes of data from the file into buf.
> > + * 'size' can be larger than the internal buffer.
> > + *
> > + * It will return size bytes unless there was an error, in which case it will
> > + * return as many as it managed to read (assuming blocking fd's which
> > + * all current QEMUFile are)
> > + */
> > int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
> > {
> >     int pending = size;
> > @@ -717,7 +757,7 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
> >     while (pending > 0) {
> >         int res;
> > 
> > -        res = qemu_peek_buffer(f, buf, pending, 0);
> > +        res = qemu_peek_buffer(f, buf, MIN(pending, IO_BUF_SIZE), 0);
> >         if (res == 0) {
> >             return done;
> >         }
> > @@ -729,11 +769,16 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
> >     return done;
> > }
> > 
> > +/*
> > + * 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, int offset)
> > {
> >     int index = f->buf_index + offset;
> > 
> >     assert(!qemu_file_is_writable(f));
> > +    assert(offset < IO_BUF_SIZE);
> > 
> >     if (index >= f->buf_size) {
> >         qemu_fill_buffer(f);
> > -- 
> > 1.9.0
> > 
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK