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

On 05/08/2013 11:27 PM, Zhang Xiaohe wrote:
> 于 2013年05月09日 01:16, Eric Blake 写道:
>> On 05/08/2013 02:50 AM, qiaonuohan wrote:
>>>
>>> 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.
>>
>> No, I was serious that you need to provide 'uncompressed' as an explicit
>> enum value.  It is very annoying to toggle between four states (three
>> compression formats and a fourth state of no compression) when the
>> fourth is available only by omitting a parameter.  The default MUST be
>> 'uncompressed' for backwards-compatibility, not 'zlib'.
>>
> We'd like to make sure that we understand you precisely.
> 
> The definion is like below:
> { 'enum': 'DumpGuestMemoryFormat',
>   'data': [ 'uncompressed', 'zlib', 'lzo', 'snappy' ] }
> 
> { 'command': 'dump-guest-memory',
>   'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
>             '*length': 'int', '*format': 'DumpCompressionFormat' } }

Closer - make sure you use the same type name in both places (the enum
name 'DumpGuestMemoryFormat' is slightly nicer than the command use of
'DumpCompressionFormat'.

> 
> 'format' is optional:
> 1. when 'format' is not specified, vmcore will be in ELF format.
> 2. when 'format' is specified and its parameter is 'uncompressed',
>    vmcore will be in ELF format as well.
> 3. when 'format' is specified and its parameter is 'zlib/lzo/snappy',
>    vmcore will be in kdump-compressed format.

Correct.  Or you could use the name 'elf' instead of 'uncompressed', if
that makes more sense - as long as the enum calls out the names you are
supporting.

> 
> If this is what you suggest, then I don't think it is necessary to
> add 'uncompressed'. The backwards-compatibility is assured by case 1,
> in which the interface is exactly the same as before. So why do we
> add another parameter to do the same thing again?

Because it is nicer to apps to allow them to explicitly specify the
default.  Making 'format' optional allows older apps to still work, but
for newer apps, it is easier to ALWAYS supply the format argument than
it is to make the generation of the format argument conditional on
whether the default behavior is desired.

Trust me - I'm reviewing this as a potential user of your interface.
When I ask for a fourth enum value, it's because I want to use it.  When
you keep coming back and complaining that you don't want to provide the
fourth enum value because it is redundant, I feel like you aren't paying
attention to your user base.  I also think that implementation-wise, it
will be easier to write your code if you have an enum supplying all four
possibilities, since it is easier to write code that defaults a missing
argument to a known enum value, and then have the rest of your code deal
with enum values, than it is to write code that has to check everywhere
whether the argument is missing (for one value) vs. one of three other
enum values.

-- 
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 --]