diff mbox

infiniband-diags: saquery; reuse dump_one_mcmember_record function

Message ID 20110616111024.fde795bc.weiny2@llnl.gov (mailing list archive)
State Rejected, archived
Delegated to: Ira Weiny
Headers show

Commit Message

Ira Weiny June 16, 2011, 6:10 p.m. UTC 
From: Ira Weiny <weiny2@llnl.gov>
Date: Thu, 2 Jun 2011 00:17:21 -0700
Subject: [PATCH] infiniband-diags: saquery; reuse dump_one_mcmember_record function

Signed-off-by: Ira Weiny <weiny2@llnl.gov>
---
 src/saquery.c |   16 ++--------------
 1 files changed, 2 insertions(+), 14 deletions(-)

Comments

Hal Rosenstock June 16, 2011, 8:53 p.m. UTC | #1 
On 6/16/2011 2:10 PM, Ira Weiny wrote:
> -		printf("MCMemberRecord member dump:\n"
> -		       "\t\tMGID....................%s\n"
> -		       "\t\tMlid....................0x%X\n"
> -		       "\t\tPortGid.................%s\n"
> -		       "\t\tScopeState..............0x%X\n"
> -		       "\t\tProxyJoin...............0x%X\n"
> -		       "\t\tNodeDescription.........%s\n",
> -		       inet_ntop(AF_INET6, p_mcmr->mgid.raw, gid_str,
> -				 sizeof gid_str),
> -		       cl_ntoh16(p_mcmr->mlid),
> -		       inet_ntop(AF_INET6, p_mcmr->port_gid.raw,
> -				 gid_str2, sizeof gid_str2),
> -		       p_mcmr->scope_state, p_mcmr->proxy_join, node_name);
> +		dump_one_mcmember_record(data);

This will print more info per MC member with much of it (group related)
repeated. Is there some need for the additional info or is this just for
code reuse ? If it's the latter, I would prefer to not see it change or
add an additional parameter to dump_one_mcmember_record as to the
specific fields to print.

-- Hal

> +		printf("\t\tNodeDescription.........%s\n", node_nam

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ira Weiny June 16, 2011, 9:46 p.m. UTC | #2 
On Thu, 16 Jun 2011 13:53:52 -0700
Hal Rosenstock <hal@dev.mellanox.co.il> wrote:

> On 6/16/2011 2:10 PM, Ira Weiny wrote:
> > -		printf("MCMemberRecord member dump:\n"
> > -		       "\t\tMGID....................%s\n"
> > -		       "\t\tMlid....................0x%X\n"
> > -		       "\t\tPortGid.................%s\n"
> > -		       "\t\tScopeState..............0x%X\n"
> > -		       "\t\tProxyJoin...............0x%X\n"
> > -		       "\t\tNodeDescription.........%s\n",
> > -		       inet_ntop(AF_INET6, p_mcmr->mgid.raw, gid_str,
> > -				 sizeof gid_str),
> > -		       cl_ntoh16(p_mcmr->mlid),
> > -		       inet_ntop(AF_INET6, p_mcmr->port_gid.raw,
> > -				 gid_str2, sizeof gid_str2),
> > -		       p_mcmr->scope_state, p_mcmr->proxy_join, node_name);
> > +		dump_one_mcmember_record(data);
> 
> This will print more info per MC member with much of it (group related)
> repeated. Is there some need for the additional info or is this just for
> code reuse ?

"need"; I guess not.  But I thought it was odd that the information was different for the 2 query methods.  There is nothing in the documentation or help output which indicates the output would be different.

That said I do see what you mean regarding the amount of data returned.  I figured most users were probably grepping this output anyway, as on a large fabric both methods return a sizable amount of data.

> If it's the latter, I would prefer to not see it change or
> add an additional parameter to dump_one_mcmember_record as to the
> specific fields to print.

Yea, the most useful output of "-m" is when used with the optional MLID value.

Ok, For now I will reject the patch.  ;-)

Ira

> 
> -- Hal
> 
> > +		printf("\t\tNodeDescription.........%s\n", node_nam
>
Jason Gunthorpe June 16, 2011, 9:59 p.m. UTC | #3 
On Thu, Jun 16, 2011 at 02:46:36PM -0700, Ira Weiny wrote:

> "need"; I guess not.  But I thought it was odd that the information was different for the 2 query methods.  There is nothing in the documentation or help output which indicates the output would be different.
> 
> That said I do see what you mean regarding the amount of data
> returned.  I figured most users were probably grepping this output
> anyway, as on a large fabric both methods return a sizable amount of
> data.

One is supposed to return de-duplicated information about the groups
that exist, the other was supposed to show group member information.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ira Weiny June 16, 2011, 10:48 p.m. UTC | #4 
On Thu, 16 Jun 2011 14:59:46 -0700
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Thu, Jun 16, 2011 at 02:46:36PM -0700, Ira Weiny wrote:
> 
> > "need"; I guess not.  But I thought it was odd that the information was different for the 2 query methods.  There is nothing in the documentation or help output which indicates the output would be different.
> > 
> > That said I do see what you mean regarding the amount of data
> > returned.  I figured most users were probably grepping this output
> > anyway, as on a large fabric both methods return a sizable amount of
> > data.
> 
> One is supposed to return de-duplicated information about the groups
> that exist, the other was supposed to show group member information.

[caveat]
I am not trying to beat a dead horse...
[/caveat]

I guess it depends on what you define as "member info" and what that implies to the user. ("de-duplicated" or not.)

       -m     get  multicast  member info.  If a group is specified, limit the output to the group specified
              and print one line containing only the GUID and node  description  for  each  entry.  Example:
              saquery -m 0xc000

To me this implied "de-duplication" only occurred with the mlid specified.  But..  I'm happy.

Ira

> 
> Jason
diff mbox

Patch

diff --git a/src/saquery.c b/src/saquery.c
index 7933fec..e1ab7dd 100644
--- a/src/saquery.c
+++ b/src/saquery.c
@@ -486,7 +486,6 @@  static void dump_multicast_group_record(void *data)
 static void dump_multicast_member_record(void *data)
 {
 	char gid_str[INET6_ADDRSTRLEN];
-	char gid_str2[INET6_ADDRSTRLEN];
 	ib_member_rec_t *p_mcmr = data;
 	uint16_t mlid = cl_ntoh16(p_mcmr->mlid);
 	unsigned i = 0;
@@ -512,19 +511,8 @@  static void dump_multicast_member_record(void *data)
 			       inet_ntop(AF_INET6, p_mcmr->port_gid.raw,
 					 gid_str, sizeof gid_str), node_name);
 	} else {
-		printf("MCMemberRecord member dump:\n"
-		       "\t\tMGID....................%s\n"
-		       "\t\tMlid....................0x%X\n"
-		       "\t\tPortGid.................%s\n"
-		       "\t\tScopeState..............0x%X\n"
-		       "\t\tProxyJoin...............0x%X\n"
-		       "\t\tNodeDescription.........%s\n",
-		       inet_ntop(AF_INET6, p_mcmr->mgid.raw, gid_str,
-				 sizeof gid_str),
-		       cl_ntoh16(p_mcmr->mlid),
-		       inet_ntop(AF_INET6, p_mcmr->port_gid.raw,
-				 gid_str2, sizeof gid_str2),
-		       p_mcmr->scope_state, p_mcmr->proxy_join, node_name);
+		dump_one_mcmember_record(data);
+		printf("\t\tNodeDescription.........%s\n", node_name);
 	}
 }