[openib-general] [PATCH] osm: comparing InformInfo records
Hal Rosenstock
halr at voltaire.com
Tue Nov 7 05:50:30 PST 2006
Hi Yevgeny,
On Mon, 2006-11-06 at 02:00, Yevgeny Kliteynik wrote:
> Hi Hal
>
> [From Vu Pham]
> 1. sending InformInfo set subscribe for trap 64,65,144 - this works;
> however, osm.log outputs wrong value for "subscribe" field
What code issues these subscriptions ? How was this patch tested ?
> 2. sending InformInfo set *unsubscribe* for
> trap 64,65,144 - I'm using/formating the same mad as (1) except the
> "subscribe" field is zero; however, opensm response with status 0x200
> [/From Vu Pham]
>
> 1. The received InformInfo struct was modified before dumping it.
> This was fixed as part of the second issue.
> 2. The function that compares InformInfo structures was just comparing
> the whole memory allocated for it, including reserved fields.
> Fixed to compare more selectively.
>
> Yevgeny
>
> Signed-off-by: Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il>
>
> Index: opensm/osm_sa_informinfo.c
> ===================================================================
> --- opensm/osm_sa_informinfo.c (revision 10064)
> +++ opensm/osm_sa_informinfo.c (working copy)
> @@ -345,7 +345,6 @@ osm_infr_rcv_process_set_method(
> ib_inform_info_t *p_recvd_inform_info;
> osm_infr_t inform_info_rec; /* actual inform record to be stored for reports */
> osm_infr_t *p_infr;
> - uint8_t subscribe;
> ib_net32_t qpn;
> uint8_t resp_time_val;
> ib_api_status_t res;
> @@ -403,19 +402,11 @@ osm_infr_rcv_process_set_method(
> *
> * QPN:
> * internally we keep the QPN field of the InformInfo updated
> - * so we can simply compare the entire record - when finding such.
> - * IBA spec only requires the QPN field to be filled when an unsubscribe
> - * Set(InformInfo) is done. See table 119 p 740 QPN field
> - *
> - * SUBSCRIBE:
> - * For similar reasons we change the subscribe to 0 on the
> - * inserted/searched data
> + * so we can simply compare it in the record - when finding such.
> */
>
> - subscribe = p_recvd_inform_info->subscribe;
> - if (subscribe)
> + if (p_recvd_inform_info->subscribe)
> {
> - inform_info_rec.inform_record.inform_info.subscribe = 0;
> ib_inform_info_set_qpn(
> &inform_info_rec.inform_record.inform_info,
> inform_info_rec.report_addr.addr_type.gsi.remote_qp );
> @@ -443,7 +434,7 @@ osm_infr_rcv_process_set_method(
> p_infr = osm_infr_get_by_rec( p_rcv->p_subn, p_rcv->p_log, &inform_info_rec );
>
> /* check to see if the request was for subscribe = 1 */
> - if (subscribe)
> + if (p_recvd_inform_info->subscribe)
> {
> /* validate the request for a new or update InformInfo */
> if (__validate_infr( p_rcv, &inform_info_rec ) != TRUE)
> @@ -480,6 +471,8 @@ osm_infr_rcv_process_set_method(
> goto Exit;
> }
>
> + /* set the subscribe bit to 0 before adding the record */
> + p_infr->inform_record.inform_info.subscribe = 0;
It seems odd to me to set subscribe to 0 for a subscription (rather than
when it is an unsibscription). Aren't only subscriptions kept in the
database ? Is this an artifact of the matching code ? If so, why not
change that ?
> /* Add this new osm_infr_t object to subnet object */
> osm_infr_insert_to_db( p_rcv->p_subn, p_rcv->p_log, p_infr );
>
> @@ -488,6 +481,8 @@ osm_infr_rcv_process_set_method(
> {
> /* Update the old instance of the osm_infr_t object */
> p_infr->inform_record = inform_info_rec.inform_record;
> + /* set the subscribe bit to 0 after updating the record */
> + p_infr->inform_record.inform_info.subscribe = 0;
Same as previous comment.
> }
> }
> else
> Index: opensm/osm_inform.c
> ===================================================================
> --- opensm/osm_inform.c (revision 10064)
> +++ opensm/osm_inform.c (working copy)
> @@ -206,30 +206,133 @@ __match_inf_rec(
> osm_infr_t* p_infr_rec = (osm_infr_t *)context;
> osm_infr_t* p_infr = (osm_infr_t*)p_list_item;
> osm_log_t *p_log = p_infr_rec->p_infr_rcv->p_log;
> - cl_status_t status;
> - int32_t count1, count2;
> + cl_status_t status = CL_NOT_FOUND;
> + ib_gid_t all_zero_gid;
> +
>
> OSM_LOG_ENTER( p_log, __match_inf_rec);
>
> - count1 = memcmp(&p_infr->report_addr, &p_infr_rec->report_addr,
> - sizeof(p_infr_rec->report_addr));
> - if (count1)
> - osm_log( p_log, OSM_LOG_DEBUG,
> - "__match_inf_rec: "
> - "Differ by Address\n" );
> - count2 = memcmp(
> - &p_infr->inform_record.inform_info,
> - &p_infr_rec->inform_record.inform_info,
> - sizeof(p_infr->inform_record.inform_info) );
> - if (count2)
> - osm_log( p_log, OSM_LOG_DEBUG,
> - "__match_inf_rec: "
> - "Differ by InformInfo\n" );
> - if ((count1 == 0) && (count2 == 0))
> - status = CL_SUCCESS;
> + if ( !memcmp(&p_infr->report_addr,
> + &p_infr_rec->report_addr,
> + sizeof(p_infr_rec->report_addr)) )
> + {
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by Address\n" );
> + goto Exit;
> + }
> +
> + memset(&all_zero_gid, 0, sizeof(ib_gid_t));
> +
> + /* if inform_info.gid is not zero, ignoring lid range */
> + if ( !memcmp(&p_infr_rec->inform_record.inform_info.gid,
> + &all_zero_gid,
> + sizeof(p_infr_rec->inform_record.inform_info.gid)) )
> + {
> + if ( !memcmp(&p_infr->inform_record.inform_info.gid,
> + &p_infr_rec->inform_record.inform_info.gid,
> + sizeof(p_infr->inform_record.inform_info.gid)) )
> + {
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.gid\n" );
> + goto Exit;
> + }
> + }
> else
> - status = CL_NOT_FOUND;
> + {
> + if ( (p_infr->inform_record.inform_info.lid_range_begin !=
> + p_infr_rec->inform_record.inform_info.lid_range_begin) ||
> + (p_infr->inform_record.inform_info.lid_range_end !=
> + p_infr_rec->inform_record.inform_info.lid_range_end) )
> + {
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.LIDRange\n" );
> + goto Exit;
> + }
> + }
> +
> + if ( p_infr->inform_record.inform_info.is_generic !=
> + p_infr_rec->inform_record.inform_info.is_generic )
> + {
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.IsGeneric\n" );
> + goto Exit;
> + }
>
> + if ( p_infr->inform_record.inform_info.trap_type !=
> + p_infr_rec->inform_record.inform_info.trap_type )
> + {
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.TrapType\n" );
> + goto Exit;
> + }
> +
> + if ( p_infr->inform_record.inform_info.is_generic !=
> + p_infr_rec->inform_record.inform_info.is_generic )
> + {
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.IsGeneric\n" );
> + }
This appears to be a duplicate of what was added shortly earlier in this
patch.
-- Hal
> + else if (p_infr->inform_record.inform_info.is_generic)
> + {
> + if ( p_infr->inform_record.inform_info.g_or_v.generic.trap_num !=
> + p_infr_rec->inform_record.inform_info.g_or_v.generic.trap_num )
> + {
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.Generic.TrapNumber\n" );
> + goto Exit;
> + }
> + else if ( p_infr->inform_record.inform_info.g_or_v.generic.qpn_resp_time_val !=
> + p_infr_rec->inform_record.inform_info.g_or_v.generic.qpn_resp_time_val )
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.Generic.QPNRespTimeVal\n" );
> + else if ( p_infr->inform_record.inform_info.g_or_v.generic.node_type_msb !=
> + p_infr_rec->inform_record.inform_info.g_or_v.generic.node_type_msb )
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.Generic.NodeTypeMSB\n" );
> + else if ( p_infr->inform_record.inform_info.g_or_v.generic.node_type_lsb !=
> + p_infr_rec->inform_record.inform_info.g_or_v.generic.node_type_lsb )
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.Generic.NodeTypeLSB\n" );
> + else
> + status = CL_SUCCESS;
> + }
> + else
> + {
> + if ( p_infr->inform_record.inform_info.g_or_v.vend.dev_id !=
> + p_infr_rec->inform_record.inform_info.g_or_v.vend.dev_id )
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.Vendor.DeviceID\n" );
> + else if ( p_infr->inform_record.inform_info.g_or_v.vend.qpn_resp_time_val !=
> + p_infr_rec->inform_record.inform_info.g_or_v.vend.qpn_resp_time_val )
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.Vendor.QPNRespTimeVal\n" );
> + else if ( p_infr->inform_record.inform_info.g_or_v.vend.vendor_id_msb !=
> + p_infr_rec->inform_record.inform_info.g_or_v.vend.vendor_id_msb )
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.Vendor.VendorIdMSB\n" );
> + else if ( p_infr->inform_record.inform_info.g_or_v.vend.vendor_id_lsb !=
> + p_infr_rec->inform_record.inform_info.g_or_v.vend.vendor_id_lsb )
> + osm_log( p_log, OSM_LOG_DEBUG,
> + "__match_inf_rec: "
> + "Differ by InformInfo.Vendor.VendorIdLSB\n" );
> + else
> + status = CL_SUCCESS;
> + }
> +
> + Exit:
> OSM_LOG_EXIT( p_log );
> return status;
> }
>
More information about the general
mailing list