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

[Qiao Nuohan]

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<address@hidden>
> > Signed-off-by: Zhang Xiaohe<address@hidden>
> > ---
>
> > -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