[ofw] RE: [PATCH] core [4/5]

[Reuven Amitai]

Hi Fab,
 
The initial prints and checks were disabled. When I enabled them I saw
that INVALID_SETTING is more suitable.
(It also stated at the function prototype as one of the possible return
codes)
I agree that clients shouldn't pass all zero parameters but it can
happen, 
and since the check is done at the control flow It could be add without
hurting performance.
 
Reuven.

________________________________

From: Fab Tillier [mailto:ftillier at windows.microsoft.com] 
Sent: Wednesday, April 09, 2008 7:51 PM
To: Reuven Amitai; ofw at lists.openfabrics.org
Subject: RE: [PATCH] core [4/5]



Hi Reuven,

 

It seems that the original checks and prints were correct (returning
INVALID_MAX_WRS if sq/rq_depth are both zero, returning INVALID_MAX_SGE
if sq/rq_sge are both zero).

 

Also, I don't think you should check for p_qp_create or ph_qp - you
should expect clients to pass proper parameters.

 

-Fab

 

From: ofw-bounces at lists.openfabrics.org
[mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Reuven Amitai
Sent: Wednesday, April 09, 2008 6:16 AM
To: ofw at lists.openfabrics.org
Subject: [ofw] [PATCH] core [4/5]

 

Hi,

 

The following patch adds qp creation sanity checks that disabled.

 

Thanks, Reuven.

 

Index: core/al/al_pd.c

===================================================================

--- core/al/al_pd.c     (revision 1047)

+++ core/al/al_pd.c     (working copy)

@@ -333,24 +333,22 @@

            AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_PD_HANDLE\n") );

            return IB_INVALID_PD_HANDLE;

      }

-

-#if 0

-     /* seems like no need in these checks */

-     if( !p_qp_create->rq_depth || !p_qp_create->sq_depth )

+     if( !p_qp_create || !ph_qp )

      {

-           AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_MAX_WRS (%d:%d)\n",

-                 p_qp_create->rq_depth, p_qp_create->sq_depth ) );

-           return IB_INVALID_MAX_WRS;

+           AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_PARAMETER\n") );

+           return IB_INVALID_PARAMETER;

      }

-

-     if( !p_qp_create->rq_sge || !p_qp_create->sq_sge)

+     if( !p_qp_create->rq_depth &&  !p_qp_create->sq_depth )

      {

-           AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_MAX_SGE (%d:%d)\n", 

-                 p_qp_create->rq_sge, p_qp_create->sq_sge ) );

-           return IB_INVALID_MAX_SGE;

+           AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_SETTING\n") );

+           return IB_INVALID_SETTING;

      }

-#endif     

+     if( !p_qp_create->rq_sge && !p_qp_create->sq_sge)

+     {

+           AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
("IB_INVALID_SETTING\n") );

+           return IB_INVALID_SETTING;

 

+     }

      if (h_pd->obj.p_ci_ca && h_pd->obj.p_ci_ca->p_pnp_attr)

      {

            if ((p_qp_create->rq_depth >
h_pd->obj.p_ci_ca->p_pnp_attr->max_wrs) ||

 

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20080409/55a864c7/attachment.html>