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

[Eric Blake]

On 07/15/2013 08:43 PM, Stefan Hajnoczi wrote:
 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.

Here's an instance where runtime introspection would come in handy -
knowing which values of an enum are actually supportable, vs. the entire
list of names but relying on decent errors for the elements that weren't
compiled in.

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



signature.asc
Description: OpenPGP digital signature

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

[Stefan Hajnoczi]

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?

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

[Qiao Nuohan]

Thanks for your comments!


On 07/16/2013 10:43 AM, Stefan Hajnoczi wrote:

@@ -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?



The 'offset' should be 64-bit in kdump format both on 32-bit and 64-bit. And I
will avoid using off_t in structure used by kdump format. Thanks for pointing
it out.

--
Regards
Qiao Nuohan

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

[Qiao Nuohan]

functions are used to write page desc and page data to vmcore.

Signed-off-by: Qiao Nuohan qiaonuo...@cn.fujitsu.com
Reviewed-by: Zhang Xiaohe zhan...@cn.fujitsu.com
---
 configure |   50 +
 dump.c|  264 +
 include/sysemu/dump.h |   16 +++
 3 files changed, 330 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 0e0adde..0927972 100755
--- a/configure
+++ b/configure
@@ -231,6 +231,8 @@ libusb=
 usb_redir=
 glx=
 zlib=yes
+lzo=no
+snappy=no
 guest_agent=yes
 want_tools=yes
 libiscsi=
@@ -912,6 +914,10 @@ for opt do
   ;;
   --disable-zlib-test) zlib=no
   ;;
+  --enable-lzo) lzo=yes
+  ;;
+  --enable-snappy) snappy=yes
+  ;;
   --enable-guest-agent) guest_agent=yes
   ;;
   --disable-guest-agent) guest_agent=no
@@ -1471,6 +1477,42 @@ fi
 libs_softmmu=$libs_softmmu -lz
 
 ##
+# lzo check
+
+if test $lzo != no ; then
+cat  $TMPC  EOF
+#include lzo/lzo1x.h
+int main(void) { lzo_version(); return 0; }
+EOF
+if compile_prog  -llzo2 ; then
+:
+else
+error_exit lzo check failed \
+Make sure to have the lzo libs and headers installed.
+fi
+
+libs_softmmu=$libs_softmmu -llzo2
+fi
+
+##
+# snappy check
+
+if test $snappy != no ; then
+cat  $TMPC  EOF
+#include snappy-c.h
+int main(void) { snappy_max_compressed_length(4096); return 0; }
+EOF
+if compile_prog  -lsnappy ; then
+:
+else
+error_exit snappy check failed \
+Make sure to have the snappy libs and headers installed.
+fi
+
+libs_softmmu=$libs_softmmu -lsnappy
+fi
+
+##
 # libseccomp check
 
 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
+
 if test $libiscsi = yes ; then
   echo CONFIG_LIBISCSI=y  $config_host_mak
 fi
diff --git a/dump.c b/dump.c
index 9537f2d..a28e162 100644
--- a/dump.c
+++ b/dump.c
@@ -25,6 +25,14 @@
 #include qapi/error.h
 #include qmp-commands.h
 
+#include zlib.h
+#ifdef CONFIG_LZO
+#include lzo/lzo1x.h
+#endif
+#ifdef CONFIG_SNAPPY
+#include snappy-c.h
+#endif
+
 static uint16_t cpu_convert_to_target16(uint16_t val, int endian)
 {
 if (endian == ELFDATA2LSB) {
@@ -87,6 +95,7 @@ typedef struct DumpState {
 off_t offset_dump_bitmap;
 off_t offset_page;
 size_t num_dumpable;
+uint32_t flag_compress;
 } DumpState;
 
 static int dump_cleanup(DumpState *s)
@@ -1113,6 +1122,261 @@ 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;
+
+/* 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;
+#endif
+
+#ifdef CONFIG_SNAPPY
+/* buf size for snappy */
+len_buf_out_snappy = 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;
+}
+
+/*
+ * search memory blocks to find the exact place of the specified page, then
+ * dump the page into buf. memory should be read page by page, or it may exceed
+ * the boundary and fail to read
+ */
+static int readmem(void *bufptr, ram_addr_t addr, size_t size, DumpState *s)
+{
+RAMBlock *block;
+
+block = s-block;
+
+while (block) {
+if ((addr = block-offset) 
+(addr + size = block-offset + block-length)) {
+memcpy(bufptr, block-host + (addr - block-offset), size);
+return 0;
+} else {
+block = QTAILQ_NEXT(block, next);
+}
+}
+
+return -1;
+}
+
+/*
+ * check if the page is all 0
+ */
+static inline bool is_zero_page(unsigned char *buf, long page_size)
+{
+size_t i;
+
+for (i = 0; i  page_size; i++) {
+if (buf[i]) {
+return false;
+}
+}
+
+return true;
+}
+
+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,