[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