[openib-general] [PATCH] osm: comparing InformInfo records
Hal Rosenstock
halr at voltaire.com
Mon Nov 13 05:05:50 PST 2006
Hi Yevgeny,
See embedded comments below.
-- Hal
________________________________
From: Yevgeny Kliteynik [mailto:kliteyn at dev.mellanox.co.il]
Sent: Sun 11/12/2006 8:31 AM
To: Hal Rosenstock
Cc: OPENIB
Subject: Re: [PATCH] osm: comparing InformInfo records
Hal Rosenstock wrote:
> 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 ?
Unfortunately, we don't have this kind of test in the testbase, so
this patch hasn't been tested.
<hnr> OK but it has been tested with some ULP, right ? (It's not just code inspection. </hnr>
>> 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 ?
You're right.
Previously the zero value was used for comparing the whole record to
the unsubscribe request (which carries 0 in the 'subscribe' field).
Now that the comparing function has been changed, no need to keep
zeroing this field.
>> /* 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.
Right, good catch.
I'll send a V2 of this patch shortly.
<hnr> I'll try to review it this week. I won't get to integrating it until the weekend or next week. Thanks. </hnr>
-- Yevgeny
> -- 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