qemu-devel
[Top][All Lists]
Advanced

[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: Ekaterina Tumanova
Subject: Re: [Qemu-devel] [PATCH v8 08/13] dump: add API to write dump header
Date: Tue, 28 Jan 2014 18:42:39 +0400
User-agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

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 :(

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).

Thirdly, the fix seems pretty simple, so why do not incorporate it
in the first place...

Regards,
Kate.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]