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

[Zhang Xiaohe <zhangxh@cn.fujitsu.com>]

于 2013年05月09日 22:51, Eric Blake 写道:
> 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.
>
I think you give a good reason and we'll add the forth enum value as
'elf'. Thanks very much for your comments.