Re: [PATCH 07 of 10] xenalyze: decode PV_HYPERCALL_V2 records

[George Dunlap <george.dunlap@eu.citrix.com>]

On 31/05/12 12:16, David Vrabel wrote:
> Newer version of Xen produce TRC_PV_HYPERCALL_V2 records instead of
> the older TRC_PV_HYPERCALL format.  This updated format doesn't
> included the IP but it does include select hypercall arguments.
>
> Signed-off-by: David Vrabel<david.vrabel@citrix.com>
>
> diff --git a/pv.h b/pv.h
> new file mode 100644
> --- /dev/null
> +++ b/pv.h
Why does this need its own file?
> +static const char *grant_table_op_cmd_to_str(uint32_t cmd)
Hmm -- this is a different style to the other lists of this type.  I 
guess I like having it in a function.
> +{
> +    const char *cmd_str[] = {
> +        "map_grant_ref", "unmap_grant_ref", "setup_table", "dump_table",
> +        "transfer", "copy", "query_size", "unmap_and_replace",
> +        "set_version", "get_status_frames", "get_version", "swap_grant_ref",
> +    };
I'm a bit wary of having stuff just in a big list like this -- it seems 
like it makes it harder to double-check that you've gotten the right 
match-up.  I'd prefer it look like hvm_event_handler_name[], where the 
number is annotated with a comment from time to time.
> +    static char buf[32];
> +
> +    if (cmd<= 11)
> +        return cmd_str[cmd];
Instead of hardcoding the number of elements, could you use some 
calculation involving sizeof() to get that automatically?  In any case, 
it should be "cmd < N", rather than "cmd <= N-1" (where N is the number 
of elements in the array).
> +        switch(op) {
> +        case HYPERCALL_mmu_update:
> +            {
I'm not a fan of indenting a brace within a case statement -- I think 
this is emacs' default C mode, but I prefer it the other way.   (Not 
sure which config option sets this, though.)

Other than that, looks good.

  -George