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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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;
>+        case XEN_VALTAB_CONRING_SIZE:
>+            console_write_val64tab(&crash_val64tab[i]);

            console_write_val64tab(crash_val64tab[i].id, 
&crash_val64tab[i].val);

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

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel