[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/1] Make qemu_peek_buffer loop until it gets
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/1] Make qemu_peek_buffer loop until it gets it's data |
Date: |
Thu, 27 Mar 2014 09:16:21 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
"Dr. David Alan Gilbert" <address@hidden> writes:
> * Markus Armbruster (address@hidden) wrote:
>> "Dr. David Alan Gilbert (git)" <address@hidden> writes:
>>
>> > From: "Dr. David Alan Gilbert" <address@hidden>
>> >
>> > 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 <address@hidden>
>> > ---
>> > 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!