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

[Eric Blake <eblake@redhat.com>]

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

On 05/07/2013 01:16 AM, Qiao Nuohan wrote:
> Make monitor command 'dump-guest-memory' dump in kdump-compressed format.
> The command's usage:
>   dump [-p] protocol [flags] [begin] [length]
> With 'flags' set, the core file will be in kdump-compress format, and without
> it, the format is ELF. 'flags' can be:
> 1. FLAG_DUMP_COMPRESS_ZLIB(0x1), compress vmcore with zlib
> 2. FLAG_DUMP_COMPRESS_LZO(0x2), compress vmcore with lzo
> 3. FLAG_DUMP_COMPRESS_SNAPPY(0x4), compress vmcore with snappy
> 
> 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 <qiaonuohan@cn.fujitsu.com>
> Signed-off-by: Zhang Xiaohe <zhangxh@cn.fujitsu.com>

> +++ b/hmp-commands.hx
> @@ -992,9 +992,10 @@ ETEXI
>  #if defined(CONFIG_HAVE_CORE_DUMP)
>      {
>          .name       = "dump-guest-memory",
> -        .args_type  = "paging:-p,filename:F,begin:i?,length:i?",
> -        .params     = "[-p] filename [begin] [length]",
> +        .args_type  = "paging:-p,filename:F,flags:i?,begin:i?,length:i?",

This is a backwards-incompatible change to HMP ('dump-guest-memory file
0' used to treat 0 as begin, now it treats it as flags), but I don't
care (we've already declared that HMP is unstable).  If you want
back-compat, then stick flags last, not in the middle.

See below about the naming and semantics (I hate the name "flags" for a
non-flags item, and making a human user learn flag bit values is a lousy
interface).  If you fix QMP to have a sane interface, then HMP should
probably have a similar change.

> +        .params     = "[-p] filename [flags] [begin] [length]",
>          .help       = "dump guest memory to file"
> +                      "\n\t\t\t flags: the type of compression"

That documentation does nothing for me.  What types are valid?

> +++ b/qapi-schema.json
> @@ -2410,6 +2410,8 @@
>  #            2. fd: the protocol starts with "fd:", and the following string
>  #               is the fd's name.
>  #
> +# @flags: #optional if specified, the format of dump file.
> +#

Missing a "(since 1.6)" tag to declare that it was added after the fact.
 We probably also ought to solve the introspection issue before adding
this feature, so that QMP clients like libvirt know when this optional
parameter is available for use.

>  # @begin: #optional if specified, the starting physical address.
>  #
>  # @length: #optional if specified, the memory size, in bytes. If you don't
> @@ -2421,8 +2423,8 @@
>  # Since: 1.2
>  ##
>  { 'command': 'dump-guest-memory',
> -  'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
> -            '*length': 'int' } }
> +  'data': { 'paging': 'bool', 'protocol': 'str', '*flags': 'int',
> +            '*begin': 'int', '*length': 'int' } }

EWWWWW - this is a LOUSY interface. Don't make '*flags' an 'int', and
for that matter, don't name it 'flags' (that implies that we can
bitwise-or multiple compressions together, but it really doesn't make
sense to do both lzo and snappy at the same time - compression really
only makes sense as a single format at a time).  Name it for what it
represents (compression type), and provide an enum that lists the valid
types.  Something like:

{ 'enum': 'DumpGuestMemoryFormat',
  'data': [ 'uncompressed', 'zlib', 'lzo', 'snappy' ] }

{ 'command': 'dump-guest-memory',
  'data': { '*format': 'DumpGuestMemoryFormat', ... }}

-- 
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: 621 bytes --]