From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59043) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uac5H-00032y-5P for qemu-devel@nongnu.org; Thu, 09 May 2013 21:25:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uac5E-0007zD-29 for qemu-devel@nongnu.org; Thu, 09 May 2013 21:25:19 -0400 Received: from [222.73.24.84] (port=47356 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uac5D-0007yu-No for qemu-devel@nongnu.org; Thu, 09 May 2013 21:25:15 -0400 Message-ID: <518C4BA6.7020302@cn.fujitsu.com> Date: Fri, 10 May 2013 09:21:42 +0800 From: Zhang Xiaohe MIME-Version: 1.0 References: <1367911007-13990-1-git-send-email-qiaonuohan@cn.fujitsu.com> <1367911007-13990-10-git-send-email-qiaonuohan@cn.fujitsu.com> <5189362E.3030707@redhat.com> <004301ce4bc9$11040970$330c1c50$@cn.fujitsu.com> <518A886D.6080105@redhat.com> <518B33BB.9010906@cn.fujitsu.com> <518BB7D7.5050002@redhat.com> In-Reply-To: <518BB7D7.5050002@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 9/9] Make monitor command 'dump-guest-memory' dump in kdump-compressed format List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qiaonuohan , qemu-devel@nongnu.org =E4=BA=8E 2013=E5=B9=B405=E6=9C=8809=E6=97=A5 22:51, Eric Blake =E5=86=99= =E9=81=93: > On 05/08/2013 11:27 PM, Zhang Xiaohe wrote: >> =E4=BA=8E 2013=E5=B9=B405=E6=9C=8809=E6=97=A5 01:16, Eric Blake =E5=86= =99=E9=81=93: >>> 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. =