| Message ID | 1452852370-11604-2-git-send-email-liang.z.li@intel.com |
|---|---|
| State | New |
| Headers | show |
Ping ... Liang > -----Original Message----- > From: Li, Liang Z > Sent: Friday, January 15, 2016 6:06 PM > To: qemu-devel@nongnu.org > Cc: quintela@redhat.com; amit.shah@redhat.com; dgilbert@redhat.com; > zhang.zhanghailiang@huawei.com; Li, Liang Z > Subject: [PATCH RESEND v2 1/2] qemu-file: Fix > qemu_put_compression_data flaw > > 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. > > Signed-off-by: Liang Li <liang.z.li@intel.com> > --- > migration/qemu-file.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/migration/qemu-file.c b/migration/qemu-file.c index > 0bbd257..b956ab6 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -606,8 +606,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 0 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 0. > */ > > ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t > size, @@ -616,7 +622,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 0; > + } > + qemu_fflush(f); > + blen = IO_BUF_SIZE - sizeof(int32_t); > + if (blen < compressBound(size)) { > + return 0; > + } > } > if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen, > (Bytef *)p, size, level) != Z_OK) { @@ -624,7 +637,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); > + } > f->buf_index += blen; > + if (f->buf_index == IO_BUF_SIZE) { > + qemu_fflush(f); > + } > return blen + sizeof(int32_t); > } > > -- > 1.9.1
"Li, Liang Z" <liang.z.li@intel.com> wrote: > Ping ... > > Liang Hi >> We should fix this flaw to make it works with writable QEMUFile. >> >> Signed-off-by: Liang Li <liang.z.li@intel.com> >> --- >> migration/qemu-file.c | 23 +++++++++++++++++++++-- >> 1 file changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c index >> 0bbd257..b956ab6 100644 >> --- a/migration/qemu-file.c >> +++ b/migration/qemu-file.c >> @@ -606,8 +606,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 0 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 0. >> */ Ok, I still don't understand _why_ f can be writable on compression code, but well. We return r when we are not able to write, right? static int do_compress_ram_page(CompressParam *param) { int bytes_sent, blen; uint8_t *p; RAMBlock *block = param->block; ram_addr_t offset = param->offset; p = block->host + (offset & TARGET_PAGE_MASK); bytes_sent = save_page_header(param->file, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE); blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE, migrate_compress_level()); bytes_sent += blen; return bytes_sent; } But we don't check for zero when doing the compression, shouldn't this give give an error? (yes, caller of do_co_compress_ram_page() don't check for zero either). >> ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t >> size, @@ -616,7 +622,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 0; >> + } >> + qemu_fflush(f); >> + blen = IO_BUF_SIZE - sizeof(int32_t); >> + if (blen < compressBound(size)) { >> + return 0; >> + } >> } >> if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen, >> (Bytef *)p, size, level) != Z_OK) { @@ -624,7 +637,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); >> + } >> f->buf_index += blen; >> + if (f->buf_index == IO_BUF_SIZE) { >> + qemu_fflush(f); >> + } >> return blen + sizeof(int32_t); I guess you are trying to do something different with the compression code (otherwise this qemu_fflush() or add_to_iovec() don't make sense), but I don't know what. With current code, the only thing that we miss is the check for errors, right? And if we want to do something else with it, could you explain? Thanks. Later, Juan.
On %D, %SN wrote: %Q %C Liang > -----Original Message----- > From: qemu-devel-bounces+liang.z.li=intel.com@nongnu.org [mailto:qemu- > devel-bounces+liang.z.li=intel.com@nongnu.org] On Behalf Of Juan Quintela > Sent: Tuesday, February 23, 2016 5:57 PM > To: Li, Liang Z > Cc: amit.shah@redhat.com; zhang.zhanghailiang@huawei.com; qemu- > devel@nongnu.org; dgilbert@redhat.com > Subject: Re: [Qemu-devel] [PATCH RESEND v2 1/2] qemu-file: Fix > qemu_put_compression_data flaw > > "Li, Liang Z" <liang.z.li@intel.com> wrote: > > Ping ... > > > > Liang > > Hi > > >> We should fix this flaw to make it works with writable QEMUFile. > >> > >> Signed-off-by: Liang Li <liang.z.li@intel.com> > >> --- > >> migration/qemu-file.c | 23 +++++++++++++++++++++-- > >> 1 file changed, 21 insertions(+), 2 deletions(-) > >> > >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c index > >> 0bbd257..b956ab6 100644 > >> --- a/migration/qemu-file.c > >> +++ b/migration/qemu-file.c > >> @@ -606,8 +606,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 0 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 0. > >> */ > > Ok, I still don't understand _why_ f can be writable on compression code, but > well. > > We return r when we are not able to write, right? > static int do_compress_ram_page(CompressParam *param) { > int bytes_sent, blen; > uint8_t *p; > RAMBlock *block = param->block; > ram_addr_t offset = param->offset; > > p = block->host + (offset & TARGET_PAGE_MASK); > > bytes_sent = save_page_header(param->file, block, offset | > RAM_SAVE_FLAG_COMPRESS_PAGE); > blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE, > migrate_compress_level()); > bytes_sent += blen; > > return bytes_sent; > } > > But we don't check for zero when doing the compression, shouldn't this give > give an error? > > (yes, caller of do_co_compress_ram_page() don't check for zero either). > > I guess you are trying to do something different with the compression code > (otherwise this qemu_fflush() or add_to_iovec() don't make sense), but I > don't know what. > > With current code, the only thing that we miss is the check for errors, right? > And if we want to do something else with it, could you explain? Thanks. > > Later, Juan. I think these two threads will help to explain why I do that. https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg02675.html https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg02674.html I just want to refine the code in the function ram_save_compressed_page(), so as to reduce some data copy. In the other hand, qemu_put_compression_data() is a common function, it maybe used by other code, but it's current implementation has some limitation, we should make it robust. Liang
"Li, Liang Z" <liang.z.li@intel.com> wrote: > On %D, %SN wrote: > %Q > > %C Nice!!!! Template answer without insntantiate the content O:-) > > Liang > > >> -----Original Message----- >> From: qemu-devel-bounces+liang.z.li=intel.com@nongnu.org [mailto:qemu- >> devel-bounces+liang.z.li=intel.com@nongnu.org] On Behalf Of Juan Quintela >> Sent: Tuesday, February 23, 2016 5:57 PM >> To: Li, Liang Z >> Cc: amit.shah@redhat.com; zhang.zhanghailiang@huawei.com; qemu- >> devel@nongnu.org; dgilbert@redhat.com >> Subject: Re: [Qemu-devel] [PATCH RESEND v2 1/2] qemu-file: Fix >> qemu_put_compression_data flaw >> >> "Li, Liang Z" <liang.z.li@intel.com> wrote: >> > Ping ... >> > >> > Liang >> >> Hi >> >> >> We should fix this flaw to make it works with writable QEMUFile. >> >> >> >> Signed-off-by: Liang Li <liang.z.li@intel.com> >> >> --- >> >> migration/qemu-file.c | 23 +++++++++++++++++++++-- >> >> 1 file changed, 21 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c index >> >> 0bbd257..b956ab6 100644 >> >> --- a/migration/qemu-file.c >> >> +++ b/migration/qemu-file.c >> >> @@ -606,8 +606,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 0 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 0. >> >> */ >> >> Ok, I still don't understand _why_ f can be writable on compression code, but >> well. >> >> We return r when we are not able to write, right? >> static int do_compress_ram_page(CompressParam *param) { >> int bytes_sent, blen; >> uint8_t *p; >> RAMBlock *block = param->block; >> ram_addr_t offset = param->offset; >> >> p = block->host + (offset & TARGET_PAGE_MASK); >> >> bytes_sent = save_page_header(param->file, block, offset | >> RAM_SAVE_FLAG_COMPRESS_PAGE); >> blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE, >> migrate_compress_level()); >> bytes_sent += blen; >> >> return bytes_sent; >> } >> But we don't check for zero when doing the compression, shouldn't this give >> give an error? You arent answering this one. I still think that we sould check for qemu_put_compression_data() return zero. That is one error. >> (yes, caller of do_co_compress_ram_page() don't check for zero either). >> > >> I guess you are trying to do something different with the compression code >> (otherwise this qemu_fflush() or add_to_iovec() don't make sense), but I >> don't know what. >> >> With current code, the only thing that we miss is the check for errors, right? >> And if we want to do something else with it, could you explain? Thanks. >> >> Later, Juan. > > I think these two threads will help to explain why I do that. > > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg02675.html > > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg02674.html They tell that they are generic functions and other code could need the new functionality. That code don't exist yet. I understand now why you want to put it as iovec, and it make sense. Can you fix the error handling and we can add this. > I just want to refine the code in the function ram_save_compressed_page(), > so as to reduce some data copy. Ok, that explains why we want to change it to use the iovec, not the writable part. But I can add both. > In the other hand, qemu_put_compression_data() is a common function, it maybe > used by other code, but it's current implementation has some limitation, we should > make it robust. Please, fix the handling of the error code (change the zero to -1 if it makes you feel better), resubmit and I will apply, ok? Thanks, Juan.
> Nice!!!! Template answer without insntantiate the content O:-) > :-), forgot to remove it. > > > > Liang > > > > > >> -----Original Message----- > >> From: qemu-devel-bounces+liang.z.li=intel.com@nongnu.org > >> [mailto:qemu- > >> devel-bounces+liang.z.li=intel.com@nongnu.org] On Behalf Of Juan > >> devel-bounces+Quintela > >> Sent: Tuesday, February 23, 2016 5:57 PM > >> To: Li, Liang Z > >> Cc: amit.shah@redhat.com; zhang.zhanghailiang@huawei.com; qemu- > >> devel@nongnu.org; dgilbert@redhat.com > >> Subject: Re: [Qemu-devel] [PATCH RESEND v2 1/2] qemu-file: Fix > >> qemu_put_compression_data flaw > >> > >> "Li, Liang Z" <liang.z.li@intel.com> wrote: > >> > Ping ... > >> > > >> > Liang > >> > >> Hi > >> > >> >> We should fix this flaw to make it works with writable QEMUFile. > >> >> > >> >> Signed-off-by: Liang Li <liang.z.li@intel.com> > >> >> --- > >> >> migration/qemu-file.c | 23 +++++++++++++++++++++-- > >> >> 1 file changed, 21 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c index > >> >> 0bbd257..b956ab6 100644 > >> >> --- a/migration/qemu-file.c > >> >> +++ b/migration/qemu-file.c > >> >> @@ -606,8 +606,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 0 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 0. > >> >> */ > >> > >> Ok, I still don't understand _why_ f can be writable on compression > >> code, but well. > >> > >> We return r when we are not able to write, right? > >> static int do_compress_ram_page(CompressParam *param) { > >> int bytes_sent, blen; > >> uint8_t *p; > >> RAMBlock *block = param->block; > >> ram_addr_t offset = param->offset; > >> > >> p = block->host + (offset & TARGET_PAGE_MASK); > >> > >> bytes_sent = save_page_header(param->file, block, offset | > >> RAM_SAVE_FLAG_COMPRESS_PAGE); > >> blen = qemu_put_compression_data(param->file, p, > TARGET_PAGE_SIZE, > >> migrate_compress_level()); > >> bytes_sent += blen; > >> > >> return bytes_sent; > >> } > > >> But we don't check for zero when doing the compression, shouldn't > >> this give give an error? > > You arent answering this one. I still think that we sould check for > qemu_put_compression_data() return zero. That is one error. > > >> (yes, caller of do_co_compress_ram_page() don't check for zero either). > >> > > > >> I guess you are trying to do something different with the compression > >> code (otherwise this qemu_fflush() or add_to_iovec() don't make > >> sense), but I don't know what. > >> > >> With current code, the only thing that we miss is the check for errors, right? > >> And if we want to do something else with it, could you explain? Thanks. > >> > >> Later, Juan. > > > > I think these two threads will help to explain why I do that. > > > > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg02675.html > > > > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg02674.html > > They tell that they are generic functions and other code could need the new > functionality. That code don't exist yet. I understand now why you want to > put it as iovec, and it make sense. Can you fix the error handling and we can > add this. > > > I just want to refine the code in the function > > ram_save_compressed_page(), so as to reduce some data copy. > > Ok, that explains why we want to change it to use the iovec, not the writable > part. But I can add both. > > > In the other hand, qemu_put_compression_data() is a common function, > > it maybe used by other code, but it's current implementation has some > > limitation, we should make it robust. > > Please, fix the handling of the error code (change the zero to -1 if it makes > you feel better), resubmit and I will apply, ok? > > Thanks, Juan. Indeed, change the error code to -1 is better, will do it in the next version. Thanks. Liang
diff --git a/migration/qemu-file.c b/migration/qemu-file.c index 0bbd257..b956ab6 100644 --- a/migration/qemu-file.c +++ b/migration/qemu-file.c @@ -606,8 +606,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 0 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 0. */ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size, @@ -616,7 +622,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 0; + } + qemu_fflush(f); + blen = IO_BUF_SIZE - sizeof(int32_t); + if (blen < compressBound(size)) { + return 0; + } } if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen, (Bytef *)p, size, level) != Z_OK) { @@ -624,7 +637,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); + } f->buf_index += blen; + if (f->buf_index == IO_BUF_SIZE) { + qemu_fflush(f); + } return blen + sizeof(int32_t); }
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. Signed-off-by: Liang Li <liang.z.li@intel.com> --- migration/qemu-file.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)