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

[qiaonuohan]

> -----Original Message-----
> From: Eric Blake [mailto:address@hidden
> Sent: Wednesday, May 08, 2013 1:13 AM
> To: Qiao Nuohan
> Cc: address@hidden; Zhang Xiaohe
> Subject: Re: [Qemu-devel] [PATCH 9/9] Make monitor command 'dump-guest-memory'
> dump in kdump-compressed format
> 
> 
> > +        .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.

Got it.

> 
> >  # @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', ... }}

Thanks for your suggestion. I will fix it like:

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

For zlib is treated as the default compression format, and
'uncompressed' won't be an option.

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