[ofa-general] ***SPAM*** Re: [PATCH] opensm/osm_helper.c: Add more info for traps 144 and 256-259 in osm_dump_notice

[Sasha Khapyorsky]

Hi Hal,

On 07:18 Wed 18 Mar     , Hal Rosenstock wrote:
> 
> Signed-off-by: Hal Rosenstock <hal.rosenstock at gmail.com>
> 
> ---
> diff --git a/opensm/opensm/osm_helper.c b/opensm/opensm/osm_helper.c
> index b40ba0c..593bbf7 100644
> --- a/opensm/opensm/osm_helper.c
> +++ b/opensm/opensm/osm_helper.c
> @@ -2,6 +2,7 @@
>   * Copyright (c) 2004-2008 Voltaire, Inc. All rights reserved.
>   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>   *
>   * This software is available to you under a choice of one of two
>   * licenses.  You may choose to be licensed under the terms of the GNU
> @@ -1720,7 +1721,10 @@ osm_dump_notice(IN osm_log_t * const p_log,
>  {
>  	if (osm_log_is_active(p_log, log_level)) {
>  		if (ib_notice_is_generic(p_ntci)) {
> +			int i;
>  			char buff[1024];
> +			char line[BUF_SIZE];

There is no need for two buffers. See below.

> +
>  			buff[0] = '\0';
>  
>  			/* immediate data based on the trap */
> @@ -1758,11 +1762,16 @@ osm_dump_notice(IN osm_log_t * const p_log,
>  			case 144:
>  				sprintf(buff,
>  					"\t\t\t\tlid......................%u\n"
> -					"\t\t\t\tnew_cap_mask.............0x%08x\n",
> +					"\t\t\t\tlocal_changes............%d\n"

Likely %u would be more suitable here.

> +					"\t\t\t\tnew_cap_mask.............0x%08x\n"
> +					"\t\t\t\tchange_flags.............0x%x\n",
>  					cl_ntoh16(p_ntci->data_details.ntc_144.
>  						  lid),
> +					p_ntci->data_details.ntc_144.local_changes,
>  					cl_ntoh32(p_ntci->data_details.ntc_144.
> -						  new_cap_mask));
> +						  new_cap_mask),
> +					cl_ntoh16(p_ntci->data_details.ntc_144.
> +						  change_flgs));
>  				break;
>  			case 145:
>  				sprintf(buff,
> @@ -1774,6 +1783,95 @@ osm_dump_notice(IN osm_log_t * const p_log,
>  					cl_ntoh64(p_ntci->data_details.ntc_145.
>  						  new_sys_guid));
>  				break;
> +			case 256:
> +				sprintf(buff,
> +					"\t\t\t\tlid......................%u\n"
> +					"\t\t\t\tdrslid...................%u\n"
> +					"\t\t\t\tmethod...................0x%x\n"
> +					"\t\t\t\tattr_id..................0x%x\n"
> +					"\t\t\t\tattr_mod.................0x%x\n"
> +					"\t\t\t\tm_key....................0x%016" PRIx64 "\n"
> +					"\t\t\t\tdr_notice................%d\n"
> +					"\t\t\t\tdr_path_truncated........%d\n"
> +					"\t\t\t\tdr_hop_count.............%u\n",
> +					cl_ntoh16(p_ntci->data_details.ntc_256.lid),
> +					cl_ntoh16(p_ntci->data_details.ntc_256.dr_slid),
> +					p_ntci->data_details.ntc_256.method,
> +					cl_ntoh16(p_ntci->data_details.ntc_256.attr_id),
> +					cl_ntoh32(p_ntci->data_details.ntc_256.attr_mod),
> +					cl_ntoh64(p_ntci->data_details.ntc_256.mkey),
> +					p_ntci->data_details.ntc_256.dr_trunc_hop >> 7,
> +					p_ntci->data_details.ntc_256.dr_trunc_hop >> 6,
> +					p_ntci->data_details.ntc_256.dr_trunc_hop & 0x3f);
> +				sprintf(line, "Directed Path Dump of %u hop path:"
> +					"\n\t\t\t\tPath = ",
> +					p_ntci->data_details.ntc_256.dr_trunc_hop & 0x3f);
> +				strcat(buff, line);

Instead of buffer copying it is simpler to use sprintf() return value:

	n = sprintf(buf, "blah-blah...", ...);
	sprintf(buf + n, "another blah-blah...", ...);

And using snprintf() will be even safer.

> +				for (i = 0;
> +				     i <= (p_ntci->data_details.ntc_256.dr_trunc_hop & 0x3f);
> +				     i++) {
> +					if (i == 0)
> +						sprintf(line, "%d",
> +							p_ntci->data_details.ntc_256.dr_rtn_path[i]);
> +					else
> +						sprintf(line, ",%d",
> +							p_ntci->data_details.ntc_256.dr_rtn_path[i]);
> +					strcat(buff, line);
> +				}

Ditto.

> +				break;
> +			case 257:
> +			case 258:
> +				sprintf(buff,
> +					"\t\t\t\tlid1.....................%u\n"
> +					"\t\t\t\tlid2.....................%u\n"
> +					"\t\t\t\tkey......................0x%x\n"
> +					"\t\t\t\tsl.......................%d\n"
> +					"\t\t\t\tqp1......................0x%x\n"
> +					"\t\t\t\tqp2......................0x%x\n"
> +					"\t\t\t\tgid1.....................0x%016" PRIx64 " : "
> +					"0x%016" PRIx64 "\n"
> +					"\t\t\t\tgid2.....................0x%016" PRIx64 " : "
> +					"0x%016" PRIx64 "\n",

Isn't IPv6 address format preferable for GIDs printing?

Sasha