Re: [Qemu-devel] [PATCH 12/13 v7] dump: make kdump-compressed format available for 'dump-guest-memory'

[Laszlo Ersek]

On 01/23/14 16:17, Ekaterina Tumanova wrote:
> On 01/17/2014 11:46 AM, qiaonuohan wrote:
>> Make monitor command 'dump-guest-memory' be able to 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. 'kdump-zlib': kdump-compressed format, with zlib-compressed
>> 3. 'kdump-lzo': kdump-compressed format, with lzo-compressed
>> 4. 'kdump-snappy': kdump-compressed format, with snappy-compressed
>> Without 'format' being set, it is same as 'elf'. And if non-elf format is
>> specified, paging and filter is not allowed.
>>
>> Note:
>>    1. The kdump-compressed format is readable only with the crash
>> utility and
>>       makedumpfile, and it can be smaller than the ELF format because
>> of the
>>       compression support.
>>    2. The kdump-compressed format is the 6th edition.
>>
>> Signed-off-by: Qiao Nuohan <address@hidden>
>> ---
>>   dump.c           |  129
>> +++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   hmp.c            |    5 ++-
>>   qapi-schema.json |   25 ++++++++++-
>>   qmp-commands.hx  |    7 ++-
>>   4 files changed, 156 insertions(+), 10 deletions(-)
>>
>> diff --git a/dump.c b/dump.c
>> index bb03ef7..dbf4bb6 100644
>> --- a/dump.c
>> +++ b/dump.c
>> @@ -1449,6 +1449,64 @@ out:
>>       return ret;
>>   }
>>
>> +static int create_kdump_vmcore(DumpState *s)
>> +{
>> +    int ret;
>> +
>> +    /*
>> +     * the kdump-compressed format is:
>> +     *                                               File offset
>> +     *  +------------------------------------------+ 0x0
>> +     *  |    main header (struct disk_dump_header) |
>> +     *  |------------------------------------------+ block 1
>> +     *  |    sub header (struct kdump_sub_header)  |
>> +     *  |------------------------------------------+ block 2
>> +     *  |            1st-dump_bitmap               |
>> +     *  |------------------------------------------+ block 2 + X blocks
>> +     *  |            2nd-dump_bitmap               | (aligned by block)
>> +     *  |------------------------------------------+ block 2 + 2 * X
>> blocks
>> +     *  |  page desc for pfn 0 (struct page_desc)  | (aligned by block)
>> +     *  |  page desc for pfn 1 (struct page_desc)  |
>> +     *  |                    :                     |
>> +     *  |------------------------------------------| (not aligned by
>> block)
>> +     *  |         page data (pfn 0)                |
>> +     *  |         page data (pfn 1)                |
>> +     *  |                    :                     |
>> +     *  +------------------------------------------+
>> +     */
>> +
>> +    ret = write_start_flat_header(s->fd);
>> +    if (ret < 0) {
>> +        dump_error(s, "dump: failed to write start flat header.\n");
>> +        return -1;
>> +    }
>> +
>> +    ret = write_dump_header(s);
>> +    if (ret < 0) {
>> +        return -1;
>> +    }
>> +
>> +    ret = write_dump_bitmap(s);
>> +    if (ret < 0) {
>> +        return -1;
>> +    }
>> +
>> +    ret = write_dump_pages(s);
>> +    if (ret < 0) {
>> +        return -1;
>> +    }
>> +
>> +    ret = write_end_flat_header(s->fd);
>> +    if (ret < 0) {
>> +        dump_error(s, "dump: failed to write end flat header.\n");
>> +        return -1;
>> +    }
>> +
>> +    dump_completed(s);
>> +
>> +    return 0;
>> +}
>> +
>>   static ram_addr_t get_start_block(DumpState *s)
>>   {
>>       GuestPhysBlock *block;
>> @@ -1487,7 +1545,8 @@ static void get_max_mapnr(DumpState *s)
>>       }
>>   }
>>
>> -static int dump_init(DumpState *s, int fd, bool paging, bool has_filter,
>> +static int dump_init(DumpState *s, int fd, bool has_format,
>> +                     DumpGuestMemoryFormat format, bool paging, bool
>> has_filter,
>>                        int64_t begin, int64_t length, Error **errp)
>>   {
>>       CPUState *cpu;
>> @@ -1495,6 +1554,11 @@ static int dump_init(DumpState *s, int fd, bool
>> paging, bool has_filter,
>>       Error *err = NULL;
>>       int ret;
>>
>> +    /* kdump-compressed is conflict with paging and filter */
>> +    if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>> +        assert(!paging && !has_filter);
>> +    }
>> +
>>       if (runstate_is_running()) {
>>           vm_stop(RUN_STATE_SAVE_VM);
>>           s->resume = true;
>> @@ -1565,6 +1629,28 @@ static int dump_init(DumpState *s, int fd, bool
>> paging, bool has_filter,
>>       tmp = DIV_ROUND_UP(DIV_ROUND_UP(s->max_mapnr, CHAR_BIT),
>> s->page_size);
>>       s->len_dump_bitmap = tmp * s->page_size;
>>
>> +    /* init for kdump-compressed format */
>> +    if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>> +        switch (format) {
>> +        case DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB:
>> +            s->flag_compress = DUMP_DH_COMPRESSED_ZLIB;
>> +            break;
>> +
>> +        case DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO:
>> +            s->flag_compress = DUMP_DH_COMPRESSED_LZO;
>> +            break;
>> +
>> +        case DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY:
>> +            s->flag_compress = DUMP_DH_COMPRESSED_SNAPPY;
>> +            break;
>> +
>> +        default:
>> +            s->flag_compress = 0;
>> +        }
>> +
>> +        return 0;
>> +    }
>> +
>>       if (s->has_filter) {
>>           memory_mapping_filter(&s->list, s->begin, s->length);
>>       }
>> @@ -1624,14 +1710,25 @@ cleanup:
>>   }
>>
>>   void qmp_dump_guest_memory(bool paging, const char *file, bool
>> has_begin,
>> -                           int64_t begin, bool has_length, int64_t
>> length,
>> -                           Error **errp)
>> +                           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;
>>
>> +    /*
>> +     * kdump-compressed format need the whole memory dumped, so
>> paging or
>> +     * filter is not supported here.
>> +     */
>> +    if ((has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) &&
>> +        (paging || has_begin || has_length)) {
>> +        error_setg(errp, "kdump-compressed format doesn't support
>> paging or "
>> +                         "filter");
>> +        return;
>> +    }
>>       if (has_begin && !has_length) {
>>           error_set(errp, QERR_MISSING_PARAMETER, "length");
>>           return;
>> @@ -1641,6 +1738,19 @@ void qmp_dump_guest_memory(bool paging, const
>> char *file, bool has_begin,
>>           return;
>>       }
>>
>> +    /* check whether lzo/snappy is supported */
>> +#ifndef CONFIG_LZO
>> +    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO) {
>> +        error_setg(errp, "kdump-lzo is not available now");
>> +    }
>> +#endif
> 
> When kdump-lzo is not available, command still writes a dump,
> which is half smaller then uncompressed one (and is read as partial
> dump), but is much bigger then the compressed one. Is it supposed to
> behave like that?

No, it's not.

The format availability checks set the error like they should, but the
return statements are missing. Compare these lzo and snappy fmt avail
tests to the paging/filtering test just above -- after setting the
error, qmp_dump_guest_memory() has no business continuing, it must bail out.

Good catch. I missed it in the v6 review (perhaps because I already
found other warts to clean up in these parts -- at least that's what I'm
telling myself :)).

Thanks,
Laszlo