| Message ID | 1395430254-14291-1-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/21/2014 01:30 PM, Dr. David Alan Gilbert (git) 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. > > Comment qemu_peek_byte to point out it's not guaranteed to work for > +/* > + * Attempt to fill the buffer from the underlying file > + * Returns the number of bytes read, or -ve value for an error. s/-ve/negative/ - it is not an obvious abbreviation, and I only knew what it meant because you have been told to fix it in other patches.
* Eric Blake (eblake@redhat.com) wrote: > On 03/21/2014 01:30 PM, Dr. David Alan Gilbert (git) 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. > > > > Comment qemu_peek_byte to point out it's not guaranteed to work for > > > +/* > > + * Attempt to fill the buffer from the underlying file > > + * Returns the number of bytes read, or -ve value for an error. > > s/-ve/negative/ - it is not an obvious abbreviation, and I only knew > what it meant because you have been told to fix it in other patches. If I need to recut it for another reason I'll change it. However, it's a perfectly common abbreviation that's widely used (hundreds of times in the kernel source for example). Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 03/24/2014 03:52 AM, Dr. David Alan Gilbert wrote: >> s/-ve/negative/ - it is not an obvious abbreviation, and I only knew >> what it meant because you have been told to fix it in other patches. > > If I need to recut it for another reason I'll change it. > However, it's a perfectly common abbreviation that's widely used > (hundreds of times in the kernel source for example). This isn't the kernel. git grep -e '-ve\b' has only two hits, and I question the one in checkpatch.pl.
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > 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. > > 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? Are there any "size or -errno" function values? If yes, recommend to make them ssize_t. > Signed-off-by: Dr. David Alan Gilbert <dgilbert@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. Suggest to spell out that this can succeed without filling the buffer completely, and not just because when hitting EOF. > + */ > +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. > { > 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. > + */ > +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 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? > @@ -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. > + */ > +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()?
* Markus Armbruster (armbru@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > > > 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. > > > > 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 <dgilbert@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.... > 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. > > + */ > > +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. > > @@ -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. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Markus Armbruster (armbru@redhat.com) wrote: >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: >> >> > 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. >> > >> > 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 <dgilbert@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!
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. + */ +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,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. + */ +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,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. + */ +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;