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

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> 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.
>
> Ditto for qemu_peek_byte (which can only be affected due to it's
> offset).
>
> Simplify qemu_get_buffer since it can now rely on qemu_peek_buffer to
> loop.

I think this one is wrong, will explain there.


> Use size_t rather than int for size parameters, (and result for
> those functions that never return -errno).

Nice.

> -int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
> +size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
>  {
> -    int pending = size;
> -    int done = 0;
> +    size_t res;
>  
> -    while (pending > 0) {
> -        int res;
> +    res = qemu_peek_buffer(f, buf, size, 0);
>  
> -        res = qemu_peek_buffer(f, buf, pending, 0);
> -        if (res == 0) {
> -            return done;
> -        }
> -        qemu_file_skip(f, res);
> -        buf += res;
> -        pending -= res;
> -        done += res;
> -    }
> -    return done;
> +    qemu_file_skip(f, res);
> +
> +    return res;

I think this is "theoretical" (a.k.a. no user of this functionality).
As this was coded, we could receive buffers bigger than IOBUF_SIZE, with
your change, we can't.  Just maintating the loop should fix this, right?

> +    while (index >= f->buf_size) {
> +        int received = qemu_fill_buffer(f);
> +
> +        if (received <= 0) {

here, I don't know really what to do.  We just need one character, so
the 1st call to qemu_fill_buffer() gives it to us, or we are already on
problems.  i.e. no need of the while() loop.

On the other hand, having exactly the same code looks so nice.

At some point I was thinking about making qemu_peek_byte() to use
qemu_peek_buffer(), but I think that we used qemu_peek_byte() more to
justify the overhead.  I am talking from memory here.


> diff --git a/vmstate.c b/vmstate.c
> index d1f5eb0..b8e6e31 100644
> --- a/vmstate.c
> +++ b/vmstate.c
> @@ -170,7 +170,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>          }
>          size = qemu_peek_buffer(f, (uint8_t *)idstr, len, 2);
>          if (size != len) {
> -            return 0;
> +            return -EIO;
>          }
>          idstr[size] = 0;

This was coded this way on purpose.  If we don't have a valid buffer
after the subsection identifier, just let the code continue to see if it
wasn't a subsection at all.  This colud be removed one tested that we
don't allow subsections in the middle of a section, only in places where
a section can appear.

In general, very nice patch, and fixes the problem.

Later, Juan.