Re: [PATCH 1/2] net/mlx4_core: clean up cq_res_start_move_to()

On Tue, 2014-01-14 at 08:47 +0200, Jack Morgenstein wrote:

On Tue, 07 Jan 2014 14:01:18 +0100
Paul Bolle [off-list ref] wrote:

quoted

+	} else {
+		/* state == RES_CQ_HW */
+		if (r->com.state != RES_CQ_ALLOCATED)

if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED)
to protect against any bad calls to this function
(although I know that currently there are none).

So we end up with
         } else if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED) {
                 err = -EINVAL;
         } else {
                 err = 0;
         }

don't we? Which is fine with me, as GCC still is then able to correctly
analyze this function.

This also preserves the behavior of the switch statement.

quoted

 			err = -EINVAL;
-		}
+		else
+			err = 0;
+	}
 
-		if (!err) {
-			r->com.from_state = r->com.state;
-			r->com.to_state = state;
-			r->com.state = RES_CQ_BUSY;
-			if (cq)
-				*cq = r;
-		}
+	if (!err) {
+		r->com.from_state = r->com.state;
+		r->com.to_state = state;
+		r->com.state = RES_CQ_BUSY;

Please keep the "if" here.  Protects against (future) bad calls.

quoted

+		*cq = r;
 	}

There seems to be a school of thought that says it's better to trigger
an Oops if a programming error is made (in this case by passing a NULL
cq) then silently handle that (future) programming error and make
debugging harder. But, even it that school of thought really exists,
this is up to you. Besides, it's only a triviality I added to my
patches.

Thanks for the review! I hope to send in a v2 of my patches shortly.


Paul Bolle