[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 3 of 3] KEXEC: Introduce new crashtables interface

>>> On 12.03.12 at 15:53, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> Changes since v1:
>  *  Fix up naming conventions and static qualifiers.
>  *  Remove XENLOG_WARNINGs, as in the case of a crash, they are little or no
>     use. Instead, mark entries as invalid so as to reduce confusion to the
>     crashdump kernel trying to parse the table.
>  *  Change strtab setup so a failure at that point wont fail the entire kexec
>     setup.
>  *  Change the modifications to console.c to prevent marking conring and
>     conring_size as global symbols.
>  *  Add more details into crashtables.h clarifying certain information.

While you indeed added a few things, I don't see any of my remarks
against v1 having got addressed.

>  *  Add emacs local variables at the end of crashtables.h

I'd rather like to see them go away in the other public headers too.

>+#define WRITE_STRTAB_ENTRY_CONST(v, s)                      \
>+    *(unsigned long*)ptr = (v); ptr += sl;                  \
>+    memcpy(ptr, s, sizeof(s)); ptr += round_up(sizeof(s))
>+#define WRITE_STRTAB_ENTRY_VAR(v, s)                        \
>+    *(unsigned long*)ptr = (v); ptr += sl;                  \
>+    memcpy(ptr, s, strlen(s)); ptr += round_up(strlen(s)+1)

Didn't even notice in v1 that here you're using "unsigned long" too.

>+        switch( crash_val64tab[i].id )
>+        {
>+        case XEN_VALTAB_MAX_PAGE:
>+            crash_val64tab[i].val = (uint64_t)(max_page);

Pointless cast and parentheses.

>+            break;
>+            console_write_val64tab(&crash_val64tab[i]);


would eliminate the need to expose the crashval types to console.h.
I'd further suggest folding the two console related functions.

And my reservations remain: Why is this any better than the consumer
looking at the symbol table (as it would continue to need to do for all
other information it might be after).


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.