Re: [Qemu-devel] [PATCH v4 1/1] Make qemu_peek_buffer loop until it gets it's data

[陈梁]

> * ???? (address@hidden) wrote:
>> 
>> ?? 2014??4??8????????10:29??Dr. David Alan Gilbert (git) <address@hidden> 
>> ??????
>> 
>>> From: "Dr. David Alan Gilbert" <address@hidden>
>>> 
>>> Make qemu_peek_buffer repeatedly 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.
>> hmm, I also have got some errors(infrequently). Maybe It is one point.
>> Could you show some messages about the error?
> 
> I've only seen the errors in my visitor/ber world where I use the peek_buffer
> a lot more; but the one place it is used in the existing code is in
> the code to check if we have the start of a subsection; if that goes wrong
> I'm not sure what error would be produced.
> 
> Dave
In my observation, error is more likely to happen if migration thread is 
blocked too long.
BTW, xbzrle and auto convergence are disable.

Best regards
ChenLiang
> 
>> 
>> However, this patch avoids one potential issue, thanks.
>> 
>> Reviewed-by: ChenLiang <address@hidden>
>>> 
>>> Comment qemu_peek_byte to point out it's not guaranteed to work for
>>> non-continuous peeks
>>> 
>>> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
>>> ---
>>> 
>>> v4:
>>> Limit the size qemu_get_buffer passes to qemu_fill_buffer on each loop
>>> 
>>> v3:
>>> Make qemu_fill_buffer return ssize_t
>>> Remove the other size_t cleanup from this patch - I'll get back to them 
>>> later
>>> Comment additions/cleanups from review
>>> Stretch my -ve
>>> 
>>> include/migration/qemu-file.h |  5 ++++
>>> qemu-file.c                   | 53 
>>> +++++++++++++++++++++++++++++++++++++++----
>>> 2 files changed, 54 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
>>> index a191fb6..c90f529 100644
>>> --- a/include/migration/qemu-file.h
>>> +++ b/include/migration/qemu-file.h
>>> @@ -123,6 +123,11 @@ 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);
>>> +/*
>>> + * 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, int offset);
>>> int qemu_get_byte(QEMUFile *f);
>>> void qemu_file_skip(QEMUFile *f, int size);
>>> diff --git a/qemu-file.c b/qemu-file.c
>>> index e5ec798..6ae37d0 100644
>>> --- a/qemu-file.c
>>> +++ b/qemu-file.c
>>> @@ -529,7 +529,15 @@ 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 negative value for an error.
>>> + *
>>> + * Note that it can return a partially full buffer even in a not error/not 
>>> EOF
>>> + * case if the underlying file descriptor gives a short read, and that can
>>> + * happen even on a blocking fd.
>>> + */
>>> +static ssize_t qemu_fill_buffer(QEMUFile *f)
>>> {
>>>    int len;
>>>    int pending;
>>> @@ -553,6 +561,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)
>>> @@ -683,17 +693,39 @@ void qemu_file_skip(QEMUFile *f, int size)
>>>    }
>>> }
>>> 
>>> +/*
>>> + * Read 'size' bytes from file (at 'offset') into buf without moving the
>>> + * pointer.
>>> + *
>>> + * It will return size bytes unless there was an error, in which case it 
>>> will
>>> + * return as many as it managed to read (assuming blocking fd's which
>>> + * all current QEMUFile are)
>>> + */
>>> int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
>>> {
>>>    int pending;
>>>    int index;
>>> 
>>>    assert(!qemu_file_is_writable(f));
>>> +    assert(offset < IO_BUF_SIZE);
>>> +    assert(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);
>>> +
>>> +    /*
>>> +     * qemu_fill_buffer might return just a few bytes, even when there 
>>> isn't
>>> +     * an error, so loop collecting them until we get enough.
>>> +     */
>>> +    while (pending < size) {
>>> +        int received = qemu_fill_buffer(f);
>>> +
>>> +        if (received <= 0) {
>>> +            break;
>>> +        }
>>> +
>>>        index = f->buf_index + offset;
>>>        pending = f->buf_size - index;
>>>    }
>>> @@ -709,6 +741,14 @@ int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int 
>>> size, size_t offset)
>>>    return size;
>>> }
>>> 
>>> +/*
>>> + * Read 'size' bytes of data from the file into buf.
>>> + * 'size' can be larger than the internal buffer.
>>> + *
>>> + * It will return size bytes unless there was an error, in which case it 
>>> will
>>> + * return as many as it managed to read (assuming blocking fd's which
>>> + * all current QEMUFile are)
>>> + */
>>> int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
>>> {
>>>    int pending = size;
>>> @@ -717,7 +757,7 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
>>>    while (pending > 0) {
>>>        int res;
>>> 
>>> -        res = qemu_peek_buffer(f, buf, pending, 0);
>>> +        res = qemu_peek_buffer(f, buf, MIN(pending, IO_BUF_SIZE), 0);
>>>        if (res == 0) {
>>>            return done;
>>>        }
>>> @@ -729,11 +769,16 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int 
>>> size)
>>>    return done;
>>> }
>>> 
>>> +/*
>>> + * 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, int offset)
>>> {
>>>    int index = f->buf_index + offset;
>>> 
>>>    assert(!qemu_file_is_writable(f));
>>> +    assert(offset < IO_BUF_SIZE);
>>> 
>>>    if (index >= f->buf_size) {
>>>        qemu_fill_buffer(f);
>>> -- 
>>> 1.9.0
>>> 
>>> 
>> 
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK