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

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 07/06/12 12:35, George Dunlap wrote:
> 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@xxxxxxxxxx>
>>
>> diff --git a/pv.h b/pv.h
>> new file mode 100644
>> --- /dev/null
>> +++ b/pv.h
> Why does this need its own file?

I would like to see Xenalyze split into more, smaller files each with
related functionality.  This is a start.

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

Ok.

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

Ok.

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

Ok.

Also, this also doesn't add the event name so (null) is printed in the
summary.  I'll fix this up as well.

David

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