From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36041) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axt95-0000lA-UJ for qemu-devel@nongnu.org; Wed, 04 May 2016 05:31:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axt8t-0002tF-TO for qemu-devel@nongnu.org; Wed, 04 May 2016 05:30:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38687) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axt8t-0002qA-MO for qemu-devel@nongnu.org; Wed, 04 May 2016 05:30:51 -0400 From: Juan Quintela In-Reply-To: <1462333259-3237-5-git-send-email-liang.z.li@intel.com> (Liang Li's message of "Wed, 4 May 2016 11:40:58 +0800") References: <1462333259-3237-1-git-send-email-liang.z.li@intel.com> <1462333259-3237-5-git-send-email-liang.z.li@intel.com> Reply-To: quintela@redhat.com Date: Wed, 04 May 2016 11:30:37 +0200 Message-ID: <87vb2utbk2.fsf@emacs.mitica> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 4/5] qemu-file: Fix qemu_put_compression_data flaw List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Liang Li Cc: qemu-devel@nongnu.org, amit.shah@redhat.com, berrange@redhat.com, dgilbert@redhat.com Liang Li wrote: > 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. > > Suggested-by: Juan Quintela > Signed-off-by: Liang Li Code is not enough. > --- > migration/qemu-file.c | 23 +++++++++++++++++++++-- > migration/ram.c | 6 +++++- > 2 files changed, 26 insertions(+), 3 deletions(-) > > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > index 6f4a129..b0ef1f3 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -607,8 +607,14 @@ uint64_t qemu_get_be64(QEMUFile *f) > return v; > } > > -/* compress size bytes of data start at p with specific compression > +/* Compress size bytes of data start at p with specific compression > * level 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. > */ > > ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size, > @@ -617,7 +623,14 @@ 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 (!qemu_file_is_writable(f)) { > + return -1; > + } I can't yet understand why we can be writting to a qemu_file that is not writable. But well, no harm. > + qemu_fflush(f); > + blen = IO_BUF_SIZE - sizeof(int32_t); > + if (blen < compressBound(size)) { > + return -1; > + } > } > if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen, > (Bytef *)p, size, level) != Z_OK) { > @@ -625,7 +638,13 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size, > return 0; > } > qemu_put_be32(f, blen); > + if (f->ops->writev_buffer) { > + add_to_iovec(f, f->buf + f->buf_index, blen); > + } > + } > return blen + sizeof(int32_t); > } > Ok with the changes in this function. > diff --git a/migration/ram.c b/migration/ram.c > index bc34bc5..7e62d8d 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -821,7 +821,11 @@ static int do_compress_ram_page(CompressParam *param) > RAM_SAVE_FLAG_COMPRESS_PAGE); > blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE, > migrate_compress_level()); > - bytes_sent += blen; > + if (blen < 0) { > + error_report("Insufficient buffer for compressed data!"); > + } else { > + bytes_sent += blen; > + } > > return bytes_sent; > } Are you sure that this code is ok? 1st- there are two callers od do_compress_ram_page(), only one of it uses the return value 2nd- if we were not able to write the compressed page, we continue without a single error. I think that if blen is < 0, we should just abort the migration at this point (we have just sent a header and we haven't been able to send the contents under that header. Without lot of changes, I can't see how to fix it). Thanks, Juan.