[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 08/13] dump: add API to write dump header
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v8 08/13] dump: add API to write dump header |
Date: |
Tue, 28 Jan 2014 15:55:17 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20131118 Thunderbird/17.0.11 |
On 01/28/14 15:42, Ekaterina Tumanova wrote:
> On 01/28/2014 05:37 PM, Laszlo Ersek wrote:
>> Hello Ekaterina,
>>
>> On 01/28/14 12:51, Ekaterina Tumanova wrote:
>>> On 01/28/2014 10:22 AM, qiaonuohan wrote:
>>>> the functions are used to write header of kdump-compressed format to
>>>> vmcore.
>>>> Header of kdump-compressed format includes:
>>>> 1. common header: DiskDumpHeader32 / DiskDumpHeader64
>>>> 2. sub header: KdumpSubHeader32 / KdumpSubHeader64
>>>> 3. extra information: only elf notes here
>>>>
>>>> Signed-off-by: Qiao Nuohan <address@hidden>
>>>> Reviewed-by: Laszlo Ersek <address@hidden>
>>>> ---
>>>> dump.c | 223
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>> include/sysemu/dump.h | 96 +++++++++++++++++++++
>>>> 2 files changed, 319 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/dump.c b/dump.c
>>>> index 3a1944e..4b2799f 100644
>>>> --- a/dump.c
>>>> +++ b/dump.c
>>>> @@ -778,6 +778,229 @@ static int buf_write_note(const void *buf,
>>>> size_t size, void *opaque)
>>>> return 0;
>>>> }
>>>>
>>>> +/* write common header, sub header and elf note to vmcore */
>>>> +static int create_header32(DumpState *s)
>>>> +{
>>>> + int ret = 0;
>>>> + DiskDumpHeader32 *dh = NULL;
>>>> + KdumpSubHeader32 *kh = NULL;
>>>> + size_t size;
>>>> + int endian = s->dump_info.d_endian;
>>>> + uint32_t block_size;
>>>> + uint32_t sub_hdr_size;
>>>> + uint32_t bitmap_blocks;
>>>> + uint32_t status = 0;
>>>> + uint64_t offset_note;
>>>> +
>>>> + /* write common header, the version of kdump-compressed format is
>>>> 6th */
>>>> + size = sizeof(DiskDumpHeader32);
>>>> + dh = g_malloc0(size);
>>>> +
>>>> + strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
>>>> + dh->header_version = cpu_convert_to_target32(6, endian);
>>>> + block_size = s->page_size;
>>>> + dh->block_size = cpu_convert_to_target32(block_size, endian);
>>>> + sub_hdr_size = sizeof(struct KdumpSubHeader32) + s->note_size;
>>>> + sub_hdr_size = DIV_ROUND_UP(sub_hdr_size, block_size);
>>>> + dh->sub_hdr_size = cpu_convert_to_target32(sub_hdr_size, endian);
>>>> + /* dh->max_mapnr may be truncated, full 64bit is in
>>>> kh.max_mapnr_64 */
>>>> + dh->max_mapnr = cpu_convert_to_target32(MIN(s->max_mapnr,
>>>> UINT_MAX),
>>>> + endian);
>>>> + dh->nr_cpus = cpu_convert_to_target32(s->nr_cpus, endian);
>>>> + bitmap_blocks = DIV_ROUND_UP(s->len_dump_bitmap, block_size) * 2;
>>>> + dh->bitmap_blocks = cpu_convert_to_target32(bitmap_blocks,
>>>> endian);
>>>> + memcpy(&(dh->utsname.machine), "i686", 4);
>>>> +
>>>
>>> In my opinion, it's not a right thing to hardcode the architecture in
>>> arch-independent module dump.c
>>>
>>> you hardcode it here in create_header32 routine by
>>>
>>> memcpy(&(dh->utsname.machine), "i686", 4);
>>>
>>> and in create_header64 routine by
>>>
>>> memcpy(&(dh->utsname.machine), "x86_64", 6);
>>>
>>> Besides that you code actually can work on s390x arch. IMHO, target
>>> arch should be coded here.
>>>
>>> Maybe you could make use of this function: cpu_to_uname_machine.
>>
>> I seem to recall that Qiao Nuohan stated that he (*) couldn't test this
>> feature on any other architecture than i686/x86_64.
>>
>> So my proposal is, let's not block the series based on just this one
>> point. Let's review it and allow it to be merged (if there are no other
>> problems).
>>
>> Then you and/or Christian could post a small patch that allows the
>> feature to work on s390x too, checking also that your patch doesn't
>> regress it on x86_64. Perhaps Qiao Nuohan could re-test at that time for
>> regressions (on x86_64), and follow up with his (*) Tested-by.
>>
>> Does it sound acceptable?
>>
>> (*) Qiao Nuohan, could you please state if you'd like to be referred to
>> as "he" or "she" in third person; also, as "Qiao" or "Nuohan" when using
>> just one name? I apoligize but I can't figure it out :(
>>
>> Thanks,
>> Laszlo
>>
>
> Thanks you for your reply, Laszlo.
>
> Just couple of thoughts:
>
> Firstly, I already mentioned this in previous review cycle, but there
> was no answer/changes :(
I thought that Qiao had addressed this before (replying to Christian):
http://thread.gmane.org/gmane.comp.emulators.qemu/251215/focus=251227
But I agree that no consensus was reached.
> Secondly, it feels really wrong to hardcode the specific arch in
> the arch-independent file, especially because patch doesn't
> prevent this function to be compiled and used on other architectures
> (only mentions this in commit message).
I do agree with that, but it doesn't regress existing functionality (ie.
it's pure improvement on x86 and x86_64, and doesn't break existing code
on other arches).
> Thirdly, the fix seems pretty simple, so why do not incorporate it
> in the first place...
If you're implying (as before) cpu_to_uname_machine(), that per se would
break the x86_64 case (which is the primary target arch for the
feature). Namely:
- the dump header needs: x86_64
- the proposed function returns: x86-64
Note the difference between underscore (_) and hyphen (-).
Thanks
Laszlo
- [Qemu-devel] [PATCH v8 12/13] dump: make kdump-compressed format available for 'dump-guest-memory', (continued)
- [Qemu-devel] [PATCH v8 12/13] dump: make kdump-compressed format available for 'dump-guest-memory', qiaonuohan, 2014/01/28
- [Qemu-devel] [PATCH v8 10/13] dump: add APIs to operate DataCache, qiaonuohan, 2014/01/28
- [Qemu-devel] [PATCH v8 06/13] dump: add support for lzo/snappy, qiaonuohan, 2014/01/28
- [Qemu-devel] [PATCH v8 09/13] dump: add API to write dump_bitmap, qiaonuohan, 2014/01/28
- [Qemu-devel] [PATCH v8 08/13] dump: add API to write dump header, qiaonuohan, 2014/01/28
- Re: [Qemu-devel] [PATCH v8 08/13] dump: add API to write dump header, Laszlo Ersek, 2014/01/29
- Re: [Qemu-devel] [PATCH v8 08/13] dump: add API to write dump header, Ekaterina Tumanova, 2014/01/30
[Qemu-devel] [PATCH v8 04/13] dump: add API to write vmcore, qiaonuohan, 2014/01/28
[Qemu-devel] [PATCH v8 01/13] dump: const-qualify the buf of WriteCoreDumpFunction, qiaonuohan, 2014/01/28
[Qemu-devel] [PATCH v8 07/13] dump: add members to DumpState and init some of them, qiaonuohan, 2014/01/28
[Qemu-devel] [PATCH v8 05/13] dump: add API to write elf notes to buffer, qiaonuohan, 2014/01/28
Re: [Qemu-devel] [PATCH v8 00/13] Make 'dump-guest-memory' dump in kdump-compressed format, Qiao Nuohan, 2014/01/28
Re: [Qemu-devel] [PATCH v8 00/13] Make 'dump-guest-memory' dump in kdump-compressed format, Laszlo Ersek, 2014/01/29