Re: [Qemu-devel] [PATCH 1/2] qemu-file: fix flaws of qemu_put_compression_data

[Juan Quintela <quintela@redhat.com>]

"Li, Liang Z" <liang.z.li@intel.com> wrote:
>> > There are some flaws in qemu_put_compression_data, this patch tries to
>> > fix it. Now it can be used by other code.
>> >
>> > Signed-off-by: Liang Li <liang.z.li@intel.com>
>> > ---
>> >  migration/qemu-file.c | 10 +++++++++-
>> >  1 file changed, 9 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c index
>> > 0bbd257..ef9cd4a 100644
>> > --- a/migration/qemu-file.c
>> > +++ b/migration/qemu-file.c
>> > @@ -616,7 +616,9 @@ ssize_t qemu_put_compression_data(QEMUFile *f,
>> const uint8_t *p, size_t size,
>> >      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
>> >
>> >      if (blen < compressBound(size)) {
>> > -        return 0;
>> > +        if (f->ops->writev_buffer || f->ops->put_buffer) {
>> > +            qemu_fflush(f);
>> > +        }
>> >      }
>> 
>> With your change, when we arrive here:
>> 
>> - blen could still be smaller that compressBound(size), you need to
>>   recheck
>> - blen could have changed, but you don't take that in account for the
>>   following caller.
>> 
>> So, I think code has a bug?
>
> Yes, there is a bug, I should consider the case QEMUFile with empty ops.
> The right code should be like:
>
> if (blen < compressBound(size)) {
>         if (f->ops->writev_buffer || f->ops->put_buffer) {
>             qemu_fflush(f);
>         } else {
>             return 0;
>         }
> }
> ....
>
> It is enough?

No.  We need something like:

    if (blen < compressBound(size)) {
         if (!f->ops->writev_buffer && !f->ops->put_buffer) {
             return 0;
         }
         qemu_fflush(f);
         blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
         if (blen < compressBound(size)) {
             return 0;
         }
}


No?


>
> Liang
>
>
>
>> 
>> Later, Juan.