[Qemu-devel] [PATCH 0/7] cleanups for compressed dumps (kdumps)

[Qemu-devel] [PATCH 0/7] cleanups for compressed dumps (kdumps)

[Laszlo Ersek]

In March Paolo and Luiz had some comments for upstream qemu while they
were reviewing my downstream port of Qiao Nuohan's compressed
(kdump-format) dump feature. I've finally got around addressing them.

Laszlo Ersek (7):
  dump: fill in the flat header signature more pleasingly to the eye
  dump: simplify write_start_flat_header()
  dump: eliminate DumpState.page_shift ("guest's page shift")
  dump: eliminate DumpState.page_size ("guest's page size")
  dump: select header bitness based on ELF class, not ELF architecture
  dump: hoist lzo_init() from get_len_buf_out() to dump_init()
  dump: simplify get_len_buf_out()

 include/sysemu/dump.h |   8 +--
 dump.c                | 142 ++++++++++++++++++++++----------------------------
 2 files changed, 65 insertions(+), 85 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 3/7] dump: eliminate DumpState.page_shift ("guest's page shift")

[Laszlo Ersek]

Just use TARGET_PAGE_BITS.

"DumpState.page_shift" used to have type "uint32_t", while the replacement
TARGET_PAGE_BITS has type "int". Since "DumpState.page_shift" was only
used as bit shift counts in the paddr_to_pfn() and pfn_to_paddr() macros,
this is safe.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/sysemu/dump.h |  8 ++++----
 dump.c                | 10 ++++------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index efab7a3..12af557 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -20,14 +20,14 @@
 #define VERSION_FLAT_HEADER         (1)    /* version of flattened format */
 #define END_FLAG_FLAT_HEADER        (-1)
 
 #define ARCH_PFN_OFFSET             (0)
 
-#define paddr_to_pfn(X, page_shift) \
-    (((unsigned long long)(X) >> (page_shift)) - ARCH_PFN_OFFSET)
-#define pfn_to_paddr(X, page_shift) \
-    (((unsigned long long)(X) + ARCH_PFN_OFFSET) << (page_shift))
+#define paddr_to_pfn(X) \
+    (((unsigned long long)(X) >> TARGET_PAGE_BITS) - ARCH_PFN_OFFSET)
+#define pfn_to_paddr(X) \
+    (((unsigned long long)(X) + ARCH_PFN_OFFSET) << TARGET_PAGE_BITS)
 
 /*
  * flag for compressed format
  */
 #define DUMP_DH_COMPRESSED_ZLIB     (0x1)
diff --git a/dump.c b/dump.c
index 6dec8d2..cde14d9 100644
--- a/dump.c
+++ b/dump.c
@@ -89,11 +89,10 @@ typedef struct DumpState {
 
     uint8_t *note_buf;          /* buffer for notes */
     size_t note_buf_offset;     /* the writing place in note_buf */
     uint32_t nr_cpus;           /* number of guest's cpu */
     size_t page_size;           /* guest's page size */
-    uint32_t page_shift;        /* guest's page shift */
     uint64_t max_mapnr;         /* the biggest guest's phys-mem's number */
     size_t len_dump_bitmap;     /* the size of the place used to store
                                    dump_bitmap in vmcore */
     off_t offset_dump_bitmap;   /* offset of dump_bitmap part in vmcore */
     off_t offset_page;          /* offset of page part in vmcore */
@@ -1084,19 +1083,19 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
     if (!block) {
         block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
         *blockptr = block;
         assert(block->target_start % s->page_size == 0);
         assert(block->target_end % s->page_size == 0);
-        *pfnptr = paddr_to_pfn(block->target_start, s->page_shift);
+        *pfnptr = paddr_to_pfn(block->target_start);
         if (bufptr) {
             *bufptr = block->host_addr;
         }
         return true;
     }
 
     *pfnptr = *pfnptr + 1;
-    addr = pfn_to_paddr(*pfnptr, s->page_shift);
+    addr = pfn_to_paddr(*pfnptr);
 
     if ((addr >= block->target_start) &&
         (addr + s->page_size <= block->target_end)) {
         buf = block->host_addr + (addr - block->target_start);
     } else {
@@ -1106,11 +1105,11 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
         if (!block) {
             return false;
         }
         assert(block->target_start % s->page_size == 0);
         assert(block->target_end % s->page_size == 0);
-        *pfnptr = paddr_to_pfn(block->target_start, s->page_shift);
+        *pfnptr = paddr_to_pfn(block->target_start);
         buf = block->host_addr;
     }
 
     if (bufptr) {
         *bufptr = buf;
@@ -1532,11 +1531,11 @@ static ram_addr_t get_start_block(DumpState *s)
 static void get_max_mapnr(DumpState *s)
 {
     GuestPhysBlock *last_block;
 
     last_block = QTAILQ_LAST(&s->guest_phys_blocks.head, GuestPhysBlockHead);
-    s->max_mapnr = paddr_to_pfn(last_block->target_end, s->page_shift);
+    s->max_mapnr = paddr_to_pfn(last_block->target_end);
 }
 
 static int dump_init(DumpState *s, int fd, bool has_format,
                      DumpGuestMemoryFormat format, bool paging, bool has_filter,
                      int64_t begin, int64_t length, Error **errp)
@@ -1610,11 +1609,10 @@ static int dump_init(DumpState *s, int fd, bool has_format,
         qemu_get_guest_simple_memory_mapping(&s->list, &s->guest_phys_blocks);
     }
 
     s->nr_cpus = nr_cpus;
     s->page_size = TARGET_PAGE_SIZE;
-    s->page_shift = ffs(s->page_size) - 1;
 
     get_max_mapnr(s);
 
     uint64_t tmp;
     tmp = DIV_ROUND_UP(DIV_ROUND_UP(s->max_mapnr, CHAR_BIT), s->page_size);
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/7] dump: eliminate DumpState.page_size ("guest's page size")

[Laszlo Ersek]

Use TARGET_PAGE_SIZE and ~TARGET_PAGE_MASK instead.

"DumpState.page_size" has type "size_t", whereas TARGET_PAGE_SIZE has type
"int". TARGET_PAGE_MASK is of type "int" and has negative value. The patch
affects the implicit type conversions as follows:

- create_header32() and create_header64(): assigned to "block_size", which
  has type "uint32_t". No change.

- get_next_page(): "block->target_start", "block->target_end" and "addr"
  have type "hwaddr" (uint64_t).

  Before the patch,
  - if "size_t" was "uint64_t", then no additional conversion was done as
    part of the usual arithmetic conversions,
  - If "size_t" was "uint32_t", then it was widened to uint64_t as part of
    the usual arithmetic conversions,
  for the remainder and addition operators.

  After the patch,
  - "~TARGET_PAGE_MASK" expands to  ~~((1 << TARGET_PAGE_BITS) - 1). It
    has type "int" and positive value (only least significant bits set).
    That's converted (widened) to "uint64_t" for the bit-ands. No visible
    change.
  - The same holds for the (addr + TARGET_PAGE_SIZE) addition.

- write_dump_pages():
  - TARGET_PAGE_SIZE passed as argument to a bunch of functions that all
    have prototypes. No change.

  - When incrementing "offset_data" (of type "off_t"): given that we never
    build for ILP32_OFF32 (see "-D_FILE_OFFSET_BITS=64" in configure),
    "off_t" is always "int64_t", and we only need to consider:
    - ILP32_OFFBIG: "size_t" is "uint32_t".
      - before: int64_t += uint32_t. Page size converted to int64_t for
        the addition.
      - after:  int64_t += int32_t. No change.
    - LP64_OFF64: "size_t" is "uint64_t".
      - before: int64_t += uint64_t. Offset converted to uint64_t for the
        addition, then the uint64_t result is converted to int64_t for
        storage.
      - after:  int64_t += int32_t. Same as the ILP32_OFFBIG/after case.
        No visible change.

  - (size_out < s->page_size) comparisons, and (size_out = s->page_size)
    assignment:
    - before: "size_out" is of type "size_t", no implicit conversion for
              either operator.
    - after: TARGET_PAGE_SIZE (of type "int" and positive value) is
             converted to "size_t" (for the relop because the latter is
             one of "uint32_t" and "uint64_t"). No visible change.

- dump_init():
  - DIV_ROUND_UP(DIV_ROUND_UP(s->max_mapnr, CHAR_BIT), s->page_size): The
    innermost "DumpState.max_mapnr" field has type uint64_t, which
    propagates through all implicit conversions at hand:

    #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))

    regardless of the page size macro argument's type. In the outer macro
    replacement, the page size is converted from uint32_t and int32_t
    alike to uint64_t.

  - (tmp * s->page_size) multiplication: "tmp" has size "uint64_t"; the
    RHS is converted to that type from uint32_t and int32_t just the same
    if it's not uint64_t to begin with.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 dump.c | 51 +++++++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/dump.c b/dump.c
index cde14d9..e0a606f 100644
--- a/dump.c
+++ b/dump.c
@@ -88,11 +88,10 @@ typedef struct DumpState {
     int64_t length;
 
     uint8_t *note_buf;          /* buffer for notes */
     size_t note_buf_offset;     /* the writing place in note_buf */
     uint32_t nr_cpus;           /* number of guest's cpu */
-    size_t page_size;           /* guest's page size */
     uint64_t max_mapnr;         /* the biggest guest's phys-mem's number */
     size_t len_dump_bitmap;     /* the size of the place used to store
                                    dump_bitmap in vmcore */
     off_t offset_dump_bitmap;   /* offset of dump_bitmap part in vmcore */
     off_t offset_page;          /* offset of page part in vmcore */
@@ -803,11 +802,11 @@ static int create_header32(DumpState *s)
     size = sizeof(DiskDumpHeader32);
     dh = g_malloc0(size);
 
     strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
     dh->header_version = cpu_convert_to_target32(6, endian);
-    block_size = s->page_size;
+    block_size = TARGET_PAGE_SIZE;
     dh->block_size = cpu_convert_to_target32(block_size, endian);
     sub_hdr_size = sizeof(struct KdumpSubHeader32) + s->note_size;
     sub_hdr_size = DIV_ROUND_UP(sub_hdr_size, block_size);
     dh->sub_hdr_size = cpu_convert_to_target32(sub_hdr_size, endian);
     /* dh->max_mapnr may be truncated, full 64bit is in kh.max_mapnr_64 */
@@ -910,11 +909,11 @@ static int create_header64(DumpState *s)
     size = sizeof(DiskDumpHeader64);
     dh = g_malloc0(size);
 
     strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
     dh->header_version = cpu_convert_to_target32(6, endian);
-    block_size = s->page_size;
+    block_size = TARGET_PAGE_SIZE;
     dh->block_size = cpu_convert_to_target32(block_size, endian);
     sub_hdr_size = sizeof(struct KdumpSubHeader64) + s->note_size;
     sub_hdr_size = DIV_ROUND_UP(sub_hdr_size, block_size);
     dh->sub_hdr_size = cpu_convert_to_target32(sub_hdr_size, endian);
     /* dh->max_mapnr may be truncated, full 64bit is in kh.max_mapnr_64 */
@@ -1081,12 +1080,12 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
 
     /* block == NULL means the start of the iteration */
     if (!block) {
         block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
         *blockptr = block;
-        assert(block->target_start % s->page_size == 0);
-        assert(block->target_end % s->page_size == 0);
+        assert((block->target_start & ~TARGET_PAGE_MASK) == 0);
+        assert((block->target_end & ~TARGET_PAGE_MASK) == 0);
         *pfnptr = paddr_to_pfn(block->target_start);
         if (bufptr) {
             *bufptr = block->host_addr;
         }
         return true;
@@ -1094,21 +1093,21 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
 
     *pfnptr = *pfnptr + 1;
     addr = pfn_to_paddr(*pfnptr);
 
     if ((addr >= block->target_start) &&
-        (addr + s->page_size <= block->target_end)) {
+        (addr + TARGET_PAGE_SIZE <= block->target_end)) {
         buf = block->host_addr + (addr - block->target_start);
     } else {
         /* the next page is in the next block */
         block = QTAILQ_NEXT(block, next);
         *blockptr = block;
         if (!block) {
             return false;
         }
-        assert(block->target_start % s->page_size == 0);
-        assert(block->target_end % s->page_size == 0);
+        assert((block->target_start & ~TARGET_PAGE_MASK) == 0);
+        assert((block->target_end & ~TARGET_PAGE_MASK) == 0);
         *pfnptr = paddr_to_pfn(block->target_start);
         buf = block->host_addr;
     }
 
     if (bufptr) {
@@ -1289,11 +1288,11 @@ static int write_dump_pages(DumpState *s)
 
     prepare_data_cache(&page_desc, s, offset_desc);
     prepare_data_cache(&page_data, s, offset_data);
 
     /* prepare buffer to store compressed data */
-    len_buf_out = get_len_buf_out(s->page_size, s->flag_compress);
+    len_buf_out = get_len_buf_out(TARGET_PAGE_SIZE, s->flag_compress);
     if (len_buf_out == 0) {
         dump_error(s, "dump: failed to get length of output buffer.\n");
         goto out;
     }
 
@@ -1305,31 +1304,31 @@ static int write_dump_pages(DumpState *s)
 
     /*
      * init zero page's page_desc and page_data, because every zero page
      * uses the same page_data
      */
-    pd_zero.size = cpu_convert_to_target32(s->page_size, endian);
+    pd_zero.size = cpu_convert_to_target32(TARGET_PAGE_SIZE, endian);
     pd_zero.flags = cpu_convert_to_target32(0, endian);
     pd_zero.offset = cpu_convert_to_target64(offset_data, endian);
     pd_zero.page_flags = cpu_convert_to_target64(0, endian);
-    buf = g_malloc0(s->page_size);
-    ret = write_cache(&page_data, buf, s->page_size, false);
+    buf = g_malloc0(TARGET_PAGE_SIZE);
+    ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false);
     g_free(buf);
     if (ret < 0) {
         dump_error(s, "dump: failed to write page data(zero page).\n");
         goto out;
     }
 
-    offset_data += s->page_size;
+    offset_data += TARGET_PAGE_SIZE;
 
     /*
      * dump memory to vmcore page by page. zero page will all be resided in the
      * first page of page section
      */
     while (get_next_page(&block_iter, &pfn_iter, &buf, s)) {
         /* check zero page */
-        if (is_zero_page(buf, s->page_size)) {
+        if (is_zero_page(buf, TARGET_PAGE_SIZE)) {
             ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor),
                               false);
             if (ret < 0) {
                 dump_error(s, "dump: failed to write page desc.\n");
                 goto out;
@@ -1346,12 +1345,13 @@ static int write_dump_pages(DumpState *s)
              * s->flag_compress is set. But when compression fails to work,
              * we fall back to save in plaintext.
              */
              size_out = len_buf_out;
              if ((s->flag_compress & DUMP_DH_COMPRESSED_ZLIB) &&
-                    (compress2(buf_out, (uLongf *)&size_out, buf, s->page_size,
-                    Z_BEST_SPEED) == Z_OK) && (size_out < s->page_size)) {
+                 (compress2(buf_out, (uLongf *)&size_out, buf,
+                            TARGET_PAGE_SIZE, Z_BEST_SPEED) == Z_OK) &&
+                 (size_out < TARGET_PAGE_SIZE)) {
                 pd.flags = cpu_convert_to_target32(DUMP_DH_COMPRESSED_ZLIB,
                                                    endian);
                 pd.size  = cpu_convert_to_target32(size_out, endian);
 
                 ret = write_cache(&page_data, buf_out, size_out, false);
@@ -1359,13 +1359,13 @@ static int write_dump_pages(DumpState *s)
                     dump_error(s, "dump: failed to write page data.\n");
                     goto out;
                 }
 #ifdef CONFIG_LZO
             } else if ((s->flag_compress & DUMP_DH_COMPRESSED_LZO) &&
-                    (lzo1x_1_compress(buf, s->page_size, buf_out,
+                    (lzo1x_1_compress(buf, TARGET_PAGE_SIZE, buf_out,
                     (lzo_uint *)&size_out, wrkmem) == LZO_E_OK) &&
-                    (size_out < s->page_size)) {
+                    (size_out < TARGET_PAGE_SIZE)) {
                 pd.flags = cpu_convert_to_target32(DUMP_DH_COMPRESSED_LZO,
                                                    endian);
                 pd.size  = cpu_convert_to_target32(size_out, endian);
 
                 ret = write_cache(&page_data, buf_out, size_out, false);
@@ -1374,13 +1374,13 @@ static int write_dump_pages(DumpState *s)
                     goto out;
                 }
 #endif
 #ifdef CONFIG_SNAPPY
             } else if ((s->flag_compress & DUMP_DH_COMPRESSED_SNAPPY) &&
-                    (snappy_compress((char *)buf, s->page_size,
+                    (snappy_compress((char *)buf, TARGET_PAGE_SIZE,
                     (char *)buf_out, &size_out) == SNAPPY_OK) &&
-                    (size_out < s->page_size)) {
+                    (size_out < TARGET_PAGE_SIZE)) {
                 pd.flags = cpu_convert_to_target32(
                                         DUMP_DH_COMPRESSED_SNAPPY, endian);
                 pd.size  = cpu_convert_to_target32(size_out, endian);
 
                 ret = write_cache(&page_data, buf_out, size_out, false);
@@ -1390,17 +1390,17 @@ static int write_dump_pages(DumpState *s)
                 }
 #endif
             } else {
                 /*
                  * fall back to save in plaintext, size_out should be
-                 * assigned to s->page_size
+                 * assigned TARGET_PAGE_SIZE
                  */
                 pd.flags = cpu_convert_to_target32(0, endian);
-                size_out = s->page_size;
+                size_out = TARGET_PAGE_SIZE;
                 pd.size = cpu_convert_to_target32(size_out, endian);
 
-                ret = write_cache(&page_data, buf, s->page_size, false);
+                ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false);
                 if (ret < 0) {
                     dump_error(s, "dump: failed to write page data.\n");
                     goto out;
                 }
             }
@@ -1608,17 +1608,16 @@ static int dump_init(DumpState *s, int fd, bool has_format,
     } else {
         qemu_get_guest_simple_memory_mapping(&s->list, &s->guest_phys_blocks);
     }
 
     s->nr_cpus = nr_cpus;
-    s->page_size = TARGET_PAGE_SIZE;
 
     get_max_mapnr(s);
 
     uint64_t tmp;
-    tmp = DIV_ROUND_UP(DIV_ROUND_UP(s->max_mapnr, CHAR_BIT), s->page_size);
-    s->len_dump_bitmap = tmp * s->page_size;
+    tmp = DIV_ROUND_UP(DIV_ROUND_UP(s->max_mapnr, CHAR_BIT), TARGET_PAGE_SIZE);
+    s->len_dump_bitmap = tmp * TARGET_PAGE_SIZE;
 
     /* init for kdump-compressed format */
     if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
         switch (format) {
         case DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB:
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/7] dump: select header bitness based on ELF class, not ELF architecture

[Laszlo Ersek]

The specific ELF architecture (d_machine) carries Too Much Information
(TM) for deciding between create_header32() and create_header64(), use
"d_class" instead (ELFCLASS32 vs. ELFCLASS64).

This change adapts write_dump_header() to write_elf_loads(), dump_begin()
etc. that also rely on the ELF class of the target for bitness selection.

Considering the current targets that support dumping, cpu_get_dump_info()
works as follows:
- target-s390x/arch_dump.c: (EM_S390, ELFCLASS64) only
- target-ppc/arch_dump.c (EM_PPC64, ELFCLASS64) only
- target-i386/arch_dump.c: sets (EM_X86_64, ELFCLASS64) vs. (EM_386,
  ELFCLASS32) keying off the same Long Mode Active flag.

Hence no observable change.

Approximately-suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 dump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dump.c b/dump.c
index e0a606f..7e0982b 100644
--- a/dump.c
+++ b/dump.c
@@ -998,11 +998,11 @@ out:
     return ret;
 }
 
 static int write_dump_header(DumpState *s)
 {
-    if (s->dump_info.d_machine == EM_386) {
+    if (s->dump_info.d_class == ELFCLASS32) {
         return create_header32(s);
     } else {
         return create_header64(s);
     }
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/7] dump: hoist lzo_init() from get_len_buf_out() to dump_init()

[Laszlo Ersek]

qmp_dump_guest_memory()
  dump_init()
    lzo_init() <---------+
  create_kdump_vmcore()  |
    write_dump_pages()   |
      get_len_buf_out()  |
        lzo_init() ------+

This patch doesn't change the fact that lzo_init() is called for every
LZO-compressed dump, but it makes get_len_buf_out() more focused (single
responsibility).

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 dump.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/dump.c b/dump.c
index 7e0982b..b87045e 100644
--- a/dump.c
+++ b/dump.c
@@ -1229,17 +1229,10 @@ static size_t get_len_buf_out(size_t page_size, uint32_t flag_compress)
     /* 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;
-        }
-    }
-
     /*
      * LZO will expand incompressible data by a little amount. please check the
      * following URL to see the expansion calculation:
      * http://www.oberhumer.com/opensource/lzo/lzofaq.php
      */
@@ -1623,10 +1616,16 @@ static int dump_init(DumpState *s, int fd, bool has_format,
         case DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB:
             s->flag_compress = DUMP_DH_COMPRESSED_ZLIB;
             break;
 
         case DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO:
+#ifdef CONFIG_LZO
+            if (lzo_init() != LZO_E_OK) {
+                error_setg(errp, "failed to initialize the LZO library");
+                goto cleanup;
+            }
+#endif
             s->flag_compress = DUMP_DH_COMPRESSED_LZO;
             break;
 
         case DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY:
             s->flag_compress = DUMP_DH_COMPRESSED_SNAPPY;
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/7] dump: simplify get_len_buf_out()

[Laszlo Ersek]

We can (and should) rely on the fact that s->flag_compress is exactly one
of DUMP_DH_COMPRESSED_ZLIB, DUMP_DH_COMPRESSED_LZO, and
DUMP_DH_COMPRESSED_SNAPPY.

This is ensured by the QMP schema and dump_init() in combination.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 dump.c | 44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/dump.c b/dump.c
index b87045e..97d2c8d 100644
--- a/dump.c
+++ b/dump.c
@@ -1218,39 +1218,28 @@ static void free_data_cache(DataCache *data_cache)
     g_free(data_cache->buf);
 }
 
 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;
+    switch (flag_compress) {
+    case DUMP_DH_COMPRESSED_ZLIB:
+        return compressBound(page_size);
 
-    /* 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
-    /*
-     * LZO will expand incompressible data by a little amount. please check the
-     * following URL to see the expansion calculation:
-     * http://www.oberhumer.com/opensource/lzo/lzofaq.php
-     */
-    len_buf_out_lzo = page_size + page_size / 16 + 64 + 3;
-#endif
+    case DUMP_DH_COMPRESSED_LZO:
+        /*
+         * LZO will expand incompressible data by a little amount. Please check
+         * the following URL to see the expansion calculation:
+         * http://www.oberhumer.com/opensource/lzo/lzofaq.php
+         */
+        return page_size + page_size / 16 + 64 + 3;
 
 #ifdef CONFIG_SNAPPY
-    /* buf size for snappy */
-    len_buf_out_snappy = snappy_max_compressed_length(page_size);
+    case DUMP_DH_COMPRESSED_SNAPPY:
+        return snappy_max_compressed_length(page_size);
 #endif
-
-    /* get the biggest that can store all kinds of compressed page */
-    len_buf_out = MAX(len_buf_out_zlib,
-                      MAX(len_buf_out_lzo, len_buf_out_snappy));
-
-    return len_buf_out;
+    }
+    return 0;
 }
 
 /*
  * check if the page is all 0
  */
@@ -1282,14 +1271,11 @@ static int write_dump_pages(DumpState *s)
     prepare_data_cache(&page_desc, s, offset_desc);
     prepare_data_cache(&page_data, s, offset_data);
 
     /* prepare buffer to store compressed data */
     len_buf_out = get_len_buf_out(TARGET_PAGE_SIZE, s->flag_compress);
-    if (len_buf_out == 0) {
-        dump_error(s, "dump: failed to get length of output buffer.\n");
-        goto out;
-    }
+    assert(len_buf_out != 0);
 
 #ifdef CONFIG_LZO
     wrkmem = g_malloc(LZO1X_1_MEM_COMPRESS);
 #endif
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/7] cleanups for compressed dumps (kdumps)

[Paolo Bonzini]

Il 20/05/2014 13:39, Laszlo Ersek ha scritto:
> In March Paolo and Luiz had some comments for upstream qemu while they
> were reviewing my downstream port of Qiao Nuohan's compressed
> (kdump-format) dump feature. I've finally got around addressing them.
>
> Laszlo Ersek (7):
>   dump: fill in the flat header signature more pleasingly to the eye
>   dump: simplify write_start_flat_header()
>   dump: eliminate DumpState.page_shift ("guest's page shift")
>   dump: eliminate DumpState.page_size ("guest's page size")
>   dump: select header bitness based on ELF class, not ELF architecture
>   dump: hoist lzo_init() from get_len_buf_out() to dump_init()
>   dump: simplify get_len_buf_out()
>
>  include/sysemu/dump.h |   8 +--
>  dump.c                | 142 ++++++++++++++++++++++----------------------------
>  2 files changed, 65 insertions(+), 85 deletions(-)
>

I would have a slight preference for QEMU_BUILD_BUG_ON instead of MIN in 
patch 1, but that's a really minor nit.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 0/7] cleanups for compressed dumps (kdumps)

[Laszlo Ersek]

On 05/20/14 14:56, Paolo Bonzini wrote:
> Il 20/05/2014 13:39, Laszlo Ersek ha scritto:
>> In March Paolo and Luiz had some comments for upstream qemu while they
>> were reviewing my downstream port of Qiao Nuohan's compressed
>> (kdump-format) dump feature. I've finally got around addressing them.
>>
>> Laszlo Ersek (7):
>>   dump: fill in the flat header signature more pleasingly to the eye
>>   dump: simplify write_start_flat_header()
>>   dump: eliminate DumpState.page_shift ("guest's page shift")
>>   dump: eliminate DumpState.page_size ("guest's page size")
>>   dump: select header bitness based on ELF class, not ELF architecture
>>   dump: hoist lzo_init() from get_len_buf_out() to dump_init()
>>   dump: simplify get_len_buf_out()
>>
>>  include/sysemu/dump.h |   8 +--
>>  dump.c                | 142
>> ++++++++++++++++++++++----------------------------
>>  2 files changed, 65 insertions(+), 85 deletions(-)
>>
> 
> I would have a slight preference for QEMU_BUILD_BUG_ON instead of MIN in
> patch 1, but that's a really minor nit.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks! Should I have to respin for any reason, I'll incorporate your
suggestion.

Laszlo