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

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

* Juan Quintela (quintela@redhat.com) wrote:
> "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?

Ah, actually that is a good point (I've got a feeling one of my other
worlds relies on that); yes, I'll put the loop back and fix it all to be
size_t.

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

The problem is that peek_byte takes an offset, so while qemu_fill_buffer
will get us a byte, we actually need it to get us all the bytes upto the
offset, and that's not guaranteed from one call.

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

Thanks, I'll rework and get a V2 up later.

Dave
> 
> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK