Re: [Qemu-devel] [v3 03/13] migration: Add the framework of muti-thread decompression

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]

On 12/11/2014 06:28 PM, Liang Li wrote:

In addition to David's catches:

s/muti/multi/ in the subject line.

Commit message feels sparse - is the one subject line really all we need
to know about this framework?

> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> ---
>  arch_init.c                   | 70 +++++++++++++++++++++++++++++++++++++++++++
>  include/migration/migration.h |  4 +++
>  migration.c                   | 15 ++++++++++
>  3 files changed, 89 insertions(+)
> 

>  
> +/* using compressBound() to calculate the buffer size needed to save the
> + * compressed data, to support the maximun TARGET_PAGE_SIZE bytes of
> + * data, we need more 15 bytes, use 16 to align the data.
> + */
> +#define COMPRESS_BUF_SIZE (TARGET_PAGE_SIZE + 16)
> +

>  static int ram_load(QEMUFile *f, void *opaque, int version_id)
>  {
>      int flags = 0, ret = 0;
>      static uint64_t seq_iter;
> +    int len = 0;
> +    uint8_t compbuf[COMPRESS_BUF_SIZE];
>  

Ouch - you stack-allocated more than a page of data.  This is in general
bad practice (and you should use the heap for any function that requires
more than 4k of data) because there are some architectures (cough:
windows) where exceeding the stack by more than a page risks silent
termination of the application rather than a graceful SIGSEGV (if you
can even call a stack overflow SIGSEGV graceful).  Especially true when
using helper threads, which typically have smaller stacks than the main
application.

>      seq_iter++;
>  
> @@ -1201,6 +1259,18 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>  
>              qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
>              break;
> +        case RAM_SAVE_FLAG_COMPRESS_PAGE:
> +            host = host_from_stream_offset(f, addr, flags);
> +            if (!host) {
> +                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);

s/Illegal/Invalid/ (the user isn't breaking a law, merely a constraint).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]