Re: [Xen-devel] [PATCH] xenctx: Add an option to output more registers.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.10.13 at 20:41, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
> From: Don Slutz <Don@xxxxxxxxxxxxxxx>
> 
> Also fixup handling of symbol files, and output of 2 page stacks, and output 
> of non stack memory.

Considering the size of the change this should be broken up, and
the individual changes need to be described better in the commit
message.

Without having done a thorough review, two things stand out
immediately:

> @@ -77,17 +79,24 @@ unsigned long long kernel_start = 0xffffffff80000000UL;
>  
>  static int is_kernel_text(guest_word_t addr)
>  {
> -    if (symbol_table == NULL)
> -        return (addr > kernel_start);
> +    if (symbol_table == NULL) {
> +        if (addr > kernel_start)
> +            return 2;
> +        else
> +            return 0;
> +    }
>  
>      if (addr >= kernel_stext &&
>          addr <= kernel_etext)
> -        return 1;
> -    if (addr >= kernel_hypercallpage &&
> -        addr <= kernel_hypercallpage + 4096)
> -        return 1;
> +        return 2;
> +    if (kernel_hypercallpage && (addr >= kernel_hypercallpage &&
> +                                 addr <= kernel_hypercallpage + 4096))
> +        return 2;
>      if (addr >= kernel_sinittext &&
>          addr <= kernel_einittext)
> +        return 2;
> +    if (addr >= kernel_text &&
> +        addr <= kernel_end)
>          return 1;
>      return 0;
>  }

It remains entirely unclear what distinguishes the return values
1 and 2. This needs not just an explanation in the commit message,
but rather a code comment...

> @@ -143,11 +152,11 @@ static struct symbol *lookup_symbol(guest_word_t 
> address)
>      return s->next && s->next->address <= address ? s->next : s;
>  }
>  
> -static void print_symbol(guest_word_t addr)
> +static void print_symbol(guest_word_t addr, int type)
>  {
>      struct symbol *s;
>  
> -    if (!is_kernel_text(addr))
> +    if (is_kernel_text(addr) < type)

... or, with this usage model, perhaps the return value should
rather be a self describing enum.

> @@ -615,6 +891,45 @@ static void print_stack_word(guest_word_t word, int 
> width)
>          printf(FMT_64B_WORD, word);
>  }
>  
> +static void print_mem(vcpu_guest_context_any_t *ctx, int vcpu, int width, 
> long memAddr)

"memAddr" and alike are not valid identifiers in Linux and Xen coding
style, and you make things even worse by mixing this foreign style
with our usual one inside this new function.

Jan


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