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

[Qiao Nuohan <qiaonuohan@cn.fujitsu.com>]

On 05/15/2013 10:55 AM, Eric Blake wrote:
> On 05/14/2013 08:29 PM, Qiao Nuohan wrote:
>> Make monitor command 'dump-guest-memory' dump in kdump-compressed format.
>> The command's usage:
>>    dump [-p] protocol [begin] [length] [format]
>> 'format' is used to specified the format of vmcore and can be:
>> 1. 'elf': ELF format, without compression
>> 2. 'zlib': kdump-compressed format, with zlib-compressed
>> 3. 'lzo': kdump-compressed format, with lzo-compressed
>> 4. 'snappy': kdump-compressed format, with snappy-compressed
>> And without 'format' being set, vmcore will be in ELF format.
>>
>> 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>
>> ---
>
>> -static int dump_init(DumpState *s, int fd, bool paging, bool has_filter,
>> -                     int64_t begin, int64_t length, Error **errp)
>> +static int dump_init(DumpState *s, int fd, bool compress_format,
>> +                    DumpGuestMemoryFormat format, bool paging,
>> +                    bool has_filter, int64_t begin, int64_t length, Error **errp)
>>   {
>
> Why do you need compress_format as a separate parameter, when that
> information is redundant with the contents of format?

I need this variable to decide whether in kdump-compressed format or
not. Without using it, I need to repeat "has_format && format !=
DUMP_GUEST_MEMORY_FORMAT_ELF" three times.

>
>> +void qmp_dump_guest_memory(bool paging, const char *protocol, bool has_begin,
>> +                            int64_t begin, bool has_length,
>> +                            int64_t length, bool has_format,
>> +                            DumpGuestMemoryFormat format, Error **errp)
>>   {
>>       const char *p;
>>       int fd = -1;
>>       DumpState *s;
>>       int ret;
>> +    int compress_format = 0;
>
> You are using this as a bool, so type it as a bool rather than int.

Got it.

>
>> +++ b/include/sysemu/dump.h
>> @@ -37,6 +37,13 @@
>>   #endif
>>
>>   /*
>> + * dump format
>> + */
>> +#define FLAG_DUMP_COMPRESS_ZLIB     (0x1)   /* compressed with zlib */
>> +#define FLAG_DUMP_COMPRESS_LZO      (0x2)   /* compressed with lzo */
>> +#define FLAG_DUMP_COMPRESS_SNAPPY   (0x4)   /* compressed with snappy */
>
> Why are you skipping 3?  Besides, these aren't flags that can be
> bitwise-or'd together, so the name FLAG_ is misleading.  Why not just
> make it an enum, with value 0, 1, 2 (or with 1, 2, 3)?  In fact, why
> even declare this at all, instead of reusing the DumpGuestMemoryFormat
> enum created for you by the QMP code generation from the schema?

Sorry about this, I didn't use these but forgot to remove them.

>
>> +
>> +/*
>>    * flag used in page desc of kdump-compressed format
>>    */
>>   #define DUMP_DH_COMPRESSED_ZLIB     (0x1)
>> @@ -46,6 +53,7 @@
>>   #define KDUMP_SIGNATURE             "KDUMP   "
>>   #define SIG_LEN                     (sizeof(KDUMP_SIGNATURE) - 1)
>>   #define DISKDUMP_HEADER_BLOCKS      (1)
>> +#define PAGE_SIZE                   (4096)
>
> Is that true for all platforms?

No. In "note" of patch 0/9,

"The guest should be x86 or x86_64. The other arch is not supported
now."

PAGE_SIZE here is only supposed to support x86 or x86_64.

-- 
Regards
Qiao Nuohan