Re: [Qemu-devel] [PATCH 9/9 v2] Make monitor command 'dump-guest-memory' dump in kdump-compressed format

[Eric Blake]

On 05/14/2013 08:29 PM, Qiao Nuohan wrote:
> Make monitor command 'dump-guest-memory' dump in kdump-compressed format.
> The command's usage:
>   dump [-p] protocol [begin] [length] [format]
> 'format' is used to specified the format of vmcore and can be:
> 1. 'elf': ELF format, without compression
> 2. 'zlib': kdump-compressed format, with zlib-compressed
> 3. 'lzo': kdump-compressed format, with lzo-compressed
> 4. 'snappy': kdump-compressed format, with snappy-compressed
> And without 'format' being set, vmcore will be in ELF format.
> 
> Note:
>   1. The kdump-compressed format is readable only with the crash utility, and 
> it
>      can be smaller than the ELF format because of the compression support.
>   2. The kdump-compressed format is the 5th edition.
> 
> Signed-off-by: Qiao Nuohan <address@hidden>
> Signed-off-by: Zhang Xiaohe <address@hidden>
> ---

> -static int dump_init(DumpState *s, int fd, bool paging, bool has_filter,
> -                     int64_t begin, int64_t length, Error **errp)
> +static int dump_init(DumpState *s, int fd, bool compress_format,
> +                    DumpGuestMemoryFormat format, bool paging,
> +                    bool has_filter, int64_t begin, int64_t length, Error 
> **errp)
>  {

Why do you need compress_format as a separate parameter, when that
information is redundant with the contents of format?

> +void qmp_dump_guest_memory(bool paging, const char *protocol, bool has_begin,
> +                            int64_t begin, bool has_length,
> +                            int64_t length, bool has_format,
> +                            DumpGuestMemoryFormat format, Error **errp)
>  {
>      const char *p;
>      int fd = -1;
>      DumpState *s;
>      int ret;
> +    int compress_format = 0;

You are using this as a bool, so type it as a bool rather than int.

> +++ b/include/sysemu/dump.h
> @@ -37,6 +37,13 @@
>  #endif
>  
>  /*
> + * dump format
> + */
> +#define FLAG_DUMP_COMPRESS_ZLIB     (0x1)   /* compressed with zlib */
> +#define FLAG_DUMP_COMPRESS_LZO      (0x2)   /* compressed with lzo */
> +#define FLAG_DUMP_COMPRESS_SNAPPY   (0x4)   /* compressed with snappy */

Why are you skipping 3?  Besides, these aren't flags that can be
bitwise-or'd together, so the name FLAG_ is misleading.  Why not just
make it an enum, with value 0, 1, 2 (or with 1, 2, 3)?  In fact, why
even declare this at all, instead of reusing the DumpGuestMemoryFormat
enum created for you by the QMP code generation from the schema?

> +
> +/*
>   * flag used in page desc of kdump-compressed format
>   */
>  #define DUMP_DH_COMPRESSED_ZLIB     (0x1)
> @@ -46,6 +53,7 @@
>  #define KDUMP_SIGNATURE             "KDUMP   "
>  #define SIG_LEN                     (sizeof(KDUMP_SIGNATURE) - 1)
>  #define DISKDUMP_HEADER_BLOCKS      (1)
> +#define PAGE_SIZE                   (4096)

Is that true for all platforms?

> +++ b/qapi-schema.json
> @@ -2381,6 +2381,18 @@
>  { 'command': 'device_del', 'data': {'id': 'str'} }
>  
>  ##
> +# @DumpGuestMemoryFormat
> +#
> +# Format of Guest memory dump
> +#
> +# Each represents a format of guest memory dump.
> +#
> +# Since: 1.6
> +##
> +{ 'enum': 'DumpGuestMemoryFormat',
> +  'data': [ 'elf', 'zlib', 'lzo', 'snappy' ] }

For several other enums, we document what each enum value means here.
That way, if the enum gets used more than once, you won't have to repeat...

> +
> +##
>  # @dump-guest-memory
>  #
>  # Dump guest's memory to vmcore. It is a synchronous operation that can take
> @@ -2416,13 +2428,20 @@
>  #          want to dump all guest's memory, please specify the start @begin
>  #          and @length
>  #
> +# @format: #optional if specified, the format of guest memory dump.
> +#
> +#            1. elf: dump file will be in elf format
> +#            2. zlib: dump file will be in kdump format with zlib-compressed
> +#            3. lzo: dump file will be in kdump format with lzo-compressed
> +#            4. snappy: dump file will be in kdump format with 
> snappy-compressed

...this text in each other use of the enum.  Also, if we extend the enum
in the future, you don't have to revisit quite as much documentation.

> +#
>  # Returns: nothing on success
>  #
> -# Since: 1.2
> +# Since: 1.6

Not quite right.  The dump-guest-memory command is still Since: 1.2.
Rather, it is the @format: line that should add "(since 1.6)" to show
that one additional option was added later.  Look at drive-mirror's
@buf-size for an example.

Interface is looking better, though; thanks for folding in the review
suggestions.

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

signature.asc
Description: OpenPGP digital signature