Re: [Qemu-devel] [PATCH v5 8/9] dump: Add API to write dump pages

[Stefan Hajnoczi <stefanha@gmail.com>]

On Tue, Jul 09, 2013 at 03:30:13PM +0800, Qiao Nuohan wrote:
>  if test "$seccomp" != "no" ; then
> @@ -3872,6 +3914,14 @@ if test "$glx" = "yes" ; then
>    echo "GLX_LIBS=$glx_libs" >> $config_host_mak
>  fi
>  
> +if test "$lzo" = "yes" ; then
> +  echo "CONFIG_LZO=y" >> $config_host_mak
> +fi
> +
> +if test "$snappy" = "yes" ; then
> +  echo "CONFIG_SNAPPY=y" >> $config_host_mak
> +fi

Please also include a run-time check so QEMU can produce an error when a
user chooses a compression algorithm which is not built in.  For
example, the user should get a clear error when they select Snappy but
QEMU was built without Snappy support.

> +static size_t get_len_buf_out(size_t page_size, uint32_t flag_compress)
> +{
> +    size_t len_buf_out_zlib, len_buf_out_lzo, len_buf_out_snappy;
> +    size_t len_buf_out;
> +
> +    /* init buf_out */
> +    len_buf_out_zlib = len_buf_out_lzo = len_buf_out_snappy = 0;
> +
> +    /* buf size for zlib */
> +    len_buf_out_zlib = compressBound(page_size);
> +
> +    /* buf size for lzo */
> +#ifdef CONFIG_LZO
> +    if (flag_compress & DUMP_DH_COMPRESSED_LZO) {
> +        if (lzo_init() != LZO_E_OK) {
> +            /* return 0 to indicate lzo is unavailable */
> +            return 0;
> +        }
> +    }
> +
> +    len_buf_out_lzo = page_size + page_size / 16 + 64 + 3;

Please introduce constants for these magic numbers.  I don't know what
they mean.

> +/*
> + * check if the page is all 0
> + */
> +static inline bool is_zero_page(unsigned char *buf, long page_size)

QEMU has an optimized buffer_is_zero() function which you can use
instead.

> +
> +static int write_dump_pages(DumpState *s)
> +{
> +    int ret = 0;
> +    DataCache page_desc, page_data;
> +    size_t len_buf_out, size_out;
> +    unsigned char *buf_out = NULL;
> +    off_t offset_desc, offset_data;
> +    PageDesc pd, pd_zero;
> +    uint64_t pfn_start, pfn_end, pfn;
> +    unsigned char buf[s->page_size];
> +    MemoryMapping *memory_mapping;
> +    bool zero_page;
> +
> +    prepare_data_cache(&page_desc, s);
> +    prepare_data_cache(&page_data, s);
> +
> +    /* prepare buffer to store compressed data */
> +    len_buf_out = get_len_buf_out(s->page_size, s->flag_compress);
> +    if (len_buf_out == 0) {
> +        dump_error(s, "dump: failed to get length of output buffer.\n");
> +        goto out;

This goto jumps over the declaration of wrkmem.  The g_free(wrkmem)
below will result in undefined behavior!  Please define wrkmem above and
initialize it to NULL.

> @@ -130,6 +139,13 @@ typedef struct DataCache {
>      off_t offset;       /* offset of the file */
>  } DataCache;
>  
> +typedef struct PageDesc {
> +    off_t offset;                   /* the offset of the page data*/

The guest may be 32-bit or 64-bit, independently of the QEMU host
wordsize.  Is off_t correct when running a 64-bit guest on a 32-bit
host?

I guess you are assuming off_t == uint64_t here?