| Message ID | 20191012023932.1863-2-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/compress: refine the compress case | expand |
* 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? Dave > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > --- > migration/qemu-file.c | 16 +++------------- > 1 file changed, 3 insertions(+), 13 deletions(-) > > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > index 26fb25ddc1..f3d99a96ec 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -744,11 +744,8 @@ static int qemu_compress_data(z_stream *stream, uint8_t *dest, size_t dest_len, > /* Compress size bytes of data start at p and store the compressed > * data to the buffer of f. > * > - * When f is not writable, return -1 if f has no space to save the > - * compressed data. > - * When f is wirtable and it has no space to save the compressed data, > - * do fflush first, if f still has no space to save the compressed > - * data, return -1. > + * Since the file is dummy file with empty_ops, return -1 if f has no space to > + * save the compressed data. > */ > ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream, > const uint8_t *p, size_t size) > @@ -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; > } > > blen = qemu_compress_data(stream, f->buf + f->buf_index + sizeof(int32_t), > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Nov 07, 2019 at 11:59:10AM +0000, Dr. David Alan Gilbert 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? > Hmm... sounds interesting. Toke a look into the change log, which says Current qemu_put_compression_data can only work with no writable QEMUFile, and can't work with the writable QEMUFile. But it does not provide any measure to prevent users from using it with a writable QEMUFile. We should fix this flaw to make it works with writable QEMUFile. While I don't see a chance to use writable QEMUFile. Do I miss something? >Dave >
"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.
diff --git a/migration/qemu-file.c b/migration/qemu-file.c index 26fb25ddc1..f3d99a96ec 100644 --- a/migration/qemu-file.c +++ b/migration/qemu-file.c @@ -744,11 +744,8 @@ static int qemu_compress_data(z_stream *stream, uint8_t *dest, size_t dest_len, /* Compress size bytes of data start at p and store the compressed * data to the buffer of f. * - * When f is not writable, return -1 if f has no space to save the - * compressed data. - * When f is wirtable and it has no space to save the compressed data, - * do fflush first, if f still has no space to save the compressed - * data, return -1. + * Since the file is dummy file with empty_ops, return -1 if f has no space to + * save the compressed data. */ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream, const uint8_t *p, size_t size) @@ -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; } blen = qemu_compress_data(stream, f->buf + f->buf_index + sizeof(int32_t),
We open a file with empty_ops for compress QEMUFile, which means this is not writable. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- migration/qemu-file.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)