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 <[email protected]> > --- > 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 _______________________________________________ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
