From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH 07 of 10] xenalyze: decode PV_HYPERCALL_V2 records Date: Thu, 7 Jun 2012 16:20:29 +0100 Message-ID: <4FD0C6BD.2020403@citrix.com> References: <4FD091F0.4040209@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4FD091F0.4040209@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org 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 >> >> 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