[openib-general] Re: [PATCH] Opensm - fix PathRecord get --text follows this line--

[Hal Rosenstock]

On Wed, 2005-11-30 at 07:11, Yael Kalka wrote:
> During some tests I've noticed that in PathRecord queries the SA
> doesn't compare the packetLifeTime component, if relevant comp_mask is
> turned on. This patch fixes this.

Thanks. Applied. 

A couple of comments below.

-- Hal

> Signed-off-by:  Yael Kalka <yael at mellanox.co.il>
> 
> Index: opensm/osm_sa_path_record.c
> ===================================================================
> --- opensm/osm_sa_path_record.c	(revision 4231)
> +++ opensm/osm_sa_path_record.c	(working copy)
> @@ -175,8 +175,10 @@ __osm_pr_rcv_get_path_parms(
>    ib_api_status_t          status = IB_SUCCESS;
>    uint8_t                  mtu;
>    uint8_t                  rate;
> +  uint8_t                  pkt_life;
>    uint8_t                  required_mtu;
>    uint8_t                  required_rate;
> +  uint8_t                  required_pkt_life;
>    ib_net16_t               dest_lid;
>  
>    OSM_LOG_ENTER( p_rcv->p_log, __osm_pr_rcv_get_path_parms );
> @@ -510,10 +512,54 @@ __osm_pr_rcv_get_path_parms(
>      }
>    }
>  
> +  /* Verify the pkt_life_time */
> +  /* According to spec definition Table 171 PacketLifeTime description,

Is this IBA 1.1 (rather than 1.2) ?

> +     for loopback paths, packetLifeTime shall be zero. */
> +  if ( p_src_port == p_dest_port )
> +    /* This is loopback */
> +    pkt_life = 0;
> +  else
> +    pkt_life = OSM_DEFAULT_SUBNET_TIMEOUT;
> +
> +  /* we silently ignore cases where only the PktLife selector is defined */
> +  if ((comp_mask & IB_PR_COMPMASK_PKTLIFETIMESELEC) &&
> +      (comp_mask & IB_PR_COMPMASK_PKTLIFETIME))
> +  {
> +    required_pkt_life = ib_path_rec_pkt_life( p_pr );
> +    switch( ib_path_rec_pkt_life_sel( p_pr ) )
> +    {
> +    case 0:    /* must be greater than */
> +      if( pkt_life <= required_pkt_life )
> +        status = IB_NOT_FOUND;
> +      break;
> +
> +    case 1:    /* must be less than */
> +      if( pkt_life >= required_pkt_life )
> +        status = IB_NOT_FOUND;
> +      break;

For all the selector code (not just packet life time), the less than and
greater than comparisons include =. Is that right ?

> +    case 2:    /* exact match */
> +      if( pkt_life != required_pkt_life )
> +        status = IB_NOT_FOUND;
> +      break;
> +
> +    case 3:    /* smallest available */
> +      /* can't be disqualified by this one */
> +      break;
> +
> +    default:
> +      /* if we're here, there's a bug in ib_path_rec_pkt_life_sel() */
> +      CL_ASSERT( FALSE );
> +      status = IB_ERROR;
> +      break;
> +    }
> +  }
> +
>    p_parms->mtu = mtu;
>    p_parms->rate = rate;
>    p_parms->pkey = IB_DEFAULT_PKEY;
> -  p_parms->pkt_life = OSM_DEFAULT_SUBNET_TIMEOUT;
> +  /* the pkt_life */
> +  p_parms->pkt_life = pkt_life;
>    p_parms->sl = OSM_DEFAULT_SL;
>  
>   Exit:
>