[ofa-general] Re: [PATCH] opensm: Change ib_smp_init_new to return success/failure status

[Sasha Khapyorsky]

Hi Hal,

On 09:53 Fri 31 Jul     , Hal Rosenstock wrote:
> 
> based on valid/invalid hop count rather than relying on debug assert

When could an invalid hop count be passed to this function? And what
could happen?

ib_smp_init_new() is a simple structure fill-up helper (inlined and
defined in header file) and I don't think that we need to check
parameters there.

This patch also introduces sort of inconsistency - a hop count is checked
and other parameters aren't.

> Handle invalid status appropriate in callers of ib_smp_init_new
> 
> Signed-off-by: Hal Rosenstock <hal.rosenstock at gmail.com>
> ---
> diff --git a/opensm/include/iba/ib_types.h b/opensm/include/iba/ib_types.h
> index beb7492..6668d96 100644
> --- a/opensm/include/iba/ib_types.h
> +++ b/opensm/include/iba/ib_types.h
> @@ -4091,11 +4092,11 @@ static inline boolean_t OSM_API ib_smp_is_d(IN const ib_smp_t * const p_smp)
>  *
>  * TODO
>  *	This is too big for inlining, but leave it here for now
> -*	since there is not yet another convient spot.
> +*	since there is not yet another convenient spot.
>  *
>  * SYNOPSIS
>  */
> -static inline void OSM_API
> +static inline boolean_t OSM_API
>  ib_smp_init_new(IN ib_smp_t * const p_smp,
>  		IN const uint8_t method,
>  		IN const ib_net64_t trans_id,
> @@ -4107,7 +4108,9 @@ ib_smp_init_new(IN ib_smp_t * const p_smp,
>  		IN const ib_net16_t dr_slid, IN const ib_net16_t dr_dlid)
>  {
>  	CL_ASSERT(p_smp);
> -	CL_ASSERT(hop_count < IB_SUBNET_PATH_HOPS_MAX);
> +
> +	if (hop_count >= IB_SUBNET_PATH_HOPS_MAX)
> +		return FALSE;
>  	p_smp->base_ver = 1;
>  	p_smp->mgmt_class = IB_MCLASS_SUBN_DIR;
>  	p_smp->class_ver = 1;
> @@ -4130,6 +4133,7 @@ ib_smp_init_new(IN ib_smp_t * const p_smp,
>  
>  	/* copy the path */
>  	memcpy(&p_smp->initial_path, path_out, sizeof(p_smp->initial_path));
> +	return TRUE;
>  }
>  
>  /*
> diff --git a/opensm/opensm/osm_req.c b/opensm/opensm/osm_req.c
> index be9a92b..7934173 100644
> --- a/opensm/opensm/osm_req.c
> +++ b/opensm/opensm/osm_req.c
> @@ -102,14 +102,21 @@ osm_req_get(IN osm_sm_t * sm,
>  		ib_get_sm_attr_str(attr_id), cl_ntoh16(attr_id),
>  		cl_ntoh32(attr_mod), cl_ntoh64(tid));
>  
> -	ib_smp_init_new(osm_madw_get_smp_ptr(p_madw),
> -			IB_MAD_METHOD_GET,
> -			tid,
> -			attr_id,
> -			attr_mod,
> -			p_path->hop_count,
> -			sm->p_subn->opt.m_key,
> -			p_path->path, IB_LID_PERMISSIVE, IB_LID_PERMISSIVE);
> +	if (!ib_smp_init_new(osm_madw_get_smp_ptr(p_madw),
> +			     IB_MAD_METHOD_GET,
> +			     tid,
> +			     attr_id,
> +			     attr_mod,
> +			     p_path->hop_count,
> +			     sm->p_subn->opt.m_key,
> +			     p_path->path,
> +			     IB_LID_PERMISSIVE, IB_LID_PERMISSIVE)) {
> +		OSM_LOG(sm->p_log, OSM_LOG_ERROR, "ERR 1108: "
> +			"ib_smp_init_new failed: hop count %d\n",
> +			p_path->hop_count);

This is assumption on how ib_smp_init_new() is actually implemented -
not perfect.

Sasha