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

[Fab Tillier]

Just assert that the parameters are supplied, so that someone debugging their application will find their bugs.  Runtime checks for this shouldn't be necessary.

-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 10:13 AM
To: Fab Tillier; ofw at lists.openfabrics.org
Subject: [ofw] RE: [PATCH] core [4/5]

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/3acb89fc/attachment.html>