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