"Dr. David Alan Gilbert" <dgilb...@redhat.com> writes: > * Markus Armbruster (arm...@redhat.com) wrote: >> "Dr. David Alan Gilbert (git)" <dgilb...@redhat.com> writes: >> >> > 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. >> > >> > Comment qemu_peek_byte to point out it's not guaranteed to work for >> > non-continuous peeks >> > >> > Use size_t rather than int for size parameters, (and result for >> > those functions that never return -errno). >> >> Have you considered doing this cleanup in a separate patch? > > I'd just taken the ones involved in the functions I was > changing anyway, and it seemed small enough to roll in, but yes I can > do that. > >> Are there any "size or -errno" function values? If yes, recommend to >> make them ssize_t. > > Yes there are a few. > >> >> > Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com> >> > --- >> > include/migration/qemu-file.h | 13 +++++++---- >> > qemu-file.c | 53 >> > +++++++++++++++++++++++++++++++++++-------- >> > 2 files changed, 52 insertions(+), 14 deletions(-) >> > >> > diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h >> > index a191fb6..6dd728d 100644 >> > --- a/include/migration/qemu-file.h >> > +++ b/include/migration/qemu-file.h >> > @@ -121,11 +121,16 @@ 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); >> > -int qemu_peek_byte(QEMUFile *f, int offset); >> > +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); >> > +/* >> > + * 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, size_t 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..d426136 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. >> >> Please spell out negative. The clarity gained is well worth five >> characters. > > I can see I'm going to need a mod to checkpatch for this....
Heh. Retraining fingers can be hard :) >> Suggest to spell out that this can succeed without filling the buffer >> completely, and not just because when hitting EOF. > > Yep I can clarify that. > >> > + */ >> > +static int qemu_fill_buffer(QEMUFile *f) >> >> Aha, here's a function value that could become ssize_t. But then >> QEMUFileGetBufferFunc & friends should also be changed. Feels even more >> like a separate patch. > > Yes it could; I'd not considered that but it does make more sense than > int. However, changing that will mean I also need to change more other > places, so yeh, separate patch. > >> > { >> > 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,40 @@ 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. >> >> Begs the question what it'll do when the fd doesn't block. > > At the moment the migration stream stuff is always set up to block (although > in the postcopy world that's changed and this becomes more 'interesting'), > still > if it begs that question then at the moment it's undefined, and I don't see > a reason to define it until we use it that way/figure out what the > best thing is. Yes, specifying behavior without use cases doesn't sound useful. Easy fix for the comment: turn the else-less conditional "if the underlying fd blocks" into an explicit design assumption. >> > + */ >> > +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); >> >> Off-by-one? offset + size <= IO_BUF_SIZE > > Hmm good catch. > >> If you want to guard against overflow: 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); >> > + while (pending < size) { >> > + int received = qemu_fill_buffer(f); >> > + >> > + if (received <= 0) { >> > + break; >> > + } >> > + >> > index = f->buf_index + offset; >> > pending = f->buf_size - index; >> > } >> >> Loop is useful since qemu_fill_buffer() can succeed without filling the >> buffer, and trying again can get it filled. Correct? > > Correct; I'll add a comment to make it explicit. With qemu_fill_buffer()'s contract clarified as discussed above, a comment might not be necessary. Your choice. >> > @@ -709,13 +731,20 @@ 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) >> > +/* >> > + * Read 'size' bytes of data from the file into buf. >> > + * 'size' can be larger than the internal buffer. >> > + * >> > + * 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. >> >> Begs the question what it'll do when the fd doesn't block. > > As above. > >> > +size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size) >> > { >> > - int pending = size; >> > - int done = 0; >> > + size_t pending = size; >> > + size_t done = 0; >> > >> > while (pending > 0) { >> > - int res; >> > + size_t res; >> > >> > res = qemu_peek_buffer(f, buf, pending, 0); >> > if (res == 0) { >> > @@ -729,7 +758,11 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int >> > size) >> > return done; >> > } >> > >> > -int qemu_peek_byte(QEMUFile *f, int offset) >> > +/* >> > + * 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, size_t offset) >> > { >> > int index = f->buf_index + offset; >> >> assert the offset is sane, like you did in qemu_peek_buffer()? > > Yep, can do. Thanks!