* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilb...@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilb...@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 / dgilb...@redhat.com / Manchester, UK
