| Message ID | 1395320327-16613-1-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
"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.
* 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
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> > + 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. that is not a problem. We never got a "hole" on the things that we ask for. We ask for the "next byte", or the "next bytes", so in qemu_peek_byte() we are guaranteed (with current users) that we would only have to read a single byte. I.e. qemu_peek_{buffer,byte}(....,offset, size) means that we have "already" peek until offset, and now we want size more bytes. Later, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > >> > + 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. > > that is not a problem. > > We never got a "hole" on the things that we ask for. We ask for the > "next byte", or the "next bytes", so in qemu_peek_byte() we are > guaranteed (with current users) that we would only have to read a single > byte. > > I.e. qemu_peek_{buffer,byte}(....,offset, size) means that we have > "already" peek until offset, and now we want size more bytes. Hmm ok, that is convenient but is neither documented or enforced; I'll rework my patch and at least document it, and possibly enforce it. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> >> > + 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. >> >> that is not a problem. >> >> We never got a "hole" on the things that we ask for. We ask for the >> "next byte", or the "next bytes", so in qemu_peek_byte() we are >> guaranteed (with current users) that we would only have to read a single >> byte. >> >> I.e. qemu_peek_{buffer,byte}(....,offset, size) means that we have >> "already" peek until offset, and now we want size more bytes. > > Hmm ok, that is convenient but is neither documented or enforced; > I'll rework my patch and at least document it, and possibly enforce it. thanks very much to take the time to understand it and document it O;-) Later, Juan.
diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h index a191fb6..cc45c87 100644 --- a/include/migration/qemu-file.h +++ b/include/migration/qemu-file.h @@ -121,11 +121,11 @@ 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); +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); int qemu_peek_byte(QEMUFile *f, int 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..eb388a7 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. + */ +static int qemu_fill_buffer(QEMUFile *f) { 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,41 @@ 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. + */ +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); + /* 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; } @@ -709,24 +732,15 @@ 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) +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; } int qemu_peek_byte(QEMUFile *f, int offset) @@ -735,12 +749,14 @@ int qemu_peek_byte(QEMUFile *f, int offset) assert(!qemu_file_is_writable(f)); - if (index >= f->buf_size) { - qemu_fill_buffer(f); - index = f->buf_index + offset; - if (index >= f->buf_size) { + while (index >= f->buf_size) { + int received = qemu_fill_buffer(f); + + if (received <= 0) { return 0; } + + index = f->buf_index + offset; } return f->buf[index]; } 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;