From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:45834)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1a4wHy-0006Dq-Jn
	for qemu-devel@nongnu.org; Fri, 04 Dec 2015 14:45:07 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1a4wHt-0007wg-KB
	for qemu-devel@nongnu.org; Fri, 04 Dec 2015 14:45:06 -0500
Received: from mx1.redhat.com ([209.132.183.28]:44778)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1a4wHt-0007wc-Fw
	for qemu-devel@nongnu.org; Fri, 04 Dec 2015 14:45:01 -0500
From: Juan Quintela <quintela@redhat.com>
In-Reply-To: <F2CBF3009FA73547804AE4C663CAB28E0237D56B@SHSMSX101.ccr.corp.intel.com>
	(Liang Z. Li's message of "Fri, 4 Dec 2015 14:25:52 +0000")
References: <1449201128-22779-1-git-send-email-liang.z.li@intel.com>
	<1449201128-22779-2-git-send-email-liang.z.li@intel.com>
	<87h9jyjwuk.fsf@emacs.mitica>
	<F2CBF3009FA73547804AE4C663CAB28E0237D56B@SHSMSX101.ccr.corp.intel.com>
Date: Fri, 04 Dec 2015 20:44:47 +0100
Message-ID: <877fkuj8wg.fsf@emacs.mitica>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH 1/2] qemu-file: fix flaws of
	qemu_put_compression_data
Reply-To: quintela@redhat.com
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: "Li, Liang Z" <liang.z.li@intel.com>
Cc: "amit.shah@redhat.com" <amit.shah@redhat.com>, "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>, "dgilbert@redhat.com" <dgilbert@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.