[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