Re: [PATCH 1/2] migration/compress: compress QEMUFile is not writable

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Wei Yang (richardw.yang@linux.intel.com) wrote:
>> We open a file with empty_ops for compress QEMUFile, which means this is
>> not writable.
>
> That explanation sounds reasonable; but I'm confused by the history of
> this;  the code was added by Liang Li in :
>
>   b3be289 qemu-file: Fix qemu_put_compression_data flaw
>
>   ( https://www.mail-archive.com/qemu-devel@nongnu.org/msg368974.html )
>
> with almost exactly the opposite argument;  can we figure out why?

Comment of that patch is wrong, code agrees with this patch.
The patch that went in does:

-        return 0;
+        if (!qemu_file_is_writable(f)) {
+            return -1;
+        }
+        qemu_fflush(f);
+        blen = IO_BUF_SIZE - sizeof(int32_t);
+        if (blen < compressBound(size)) {
+            return -1;
+        }

i.e. it move from return 0 to return -1 if we are not able to write, or
if we are not able to get enough space.

>> @@ -756,14 +753,7 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream,
>>      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
>>  
>>      if (blen < compressBound(size)) {
>> -        if (!qemu_file_is_writable(f)) {
>> -            return -1;
>> -        }
>> -        qemu_fflush(f);
>> -        blen = IO_BUF_SIZE - sizeof(int32_t);
>> -        if (blen < compressBound(size)) {
>> -            return -1;
>> -        }
>> +        return -1;
>>      }

This moves from trying some things to just return -1.

And patch is ok.  compression file has a file with empty_os, so
qemu_fflush() will not help at all.

Reviewed-by: Juan Quintela <quintela@redhat.com>


Thanks, Juan.