[openib-general] Re: [PATCH] sdp conditional code cleanup

[Libor Michalek]

On Sun, Feb 27, 2005 at 12:44:41PM +0200, Michael S. Tsirkin wrote:
> Quoting r. Libor Michalek <libor at topspin.com>:
> > On Thu, Feb 24, 2005 at 11:49:28PM +0200, Michael S. Tsirkin wrote:
> > > OK, now what about things like these:
> > > 
> > >         if (0 > result) {
> > > 
> > > may I change them all to
> > > 
> > >         if (result < 0) {
> > > 
> > > While being equivalent, we are testing the result, not 0.
> > > 
> > > Similiarly (although I feel somewhat less strongly about it)
> > > 
> > >         if (0 == result)
> > > and
> > >         if (NULL == conn)
> > > 
> > > are better off as
> > > 
> > > 	if (!result) {
> > > and
> > >         if (!conn)
> > > 
> > > C is a Spartan language, and this is more brief.
> > > Libor, I think I asked about the second one, but dont recall you
> > > answering.
> > > If OK to both, let me know and I'll do it on Sunday.
> > 
> >   I actually feel more strongly in favour of making the second change
> > you propose then the first. However, I'm OK with both, so feel free
> > to submit a patch.
> 
> Here is the patch.
> 
> I generalized the approach - 
> any if (CONSTANT == variable) and if ( CONSTANT & variable) is now
>     if (variable == CONSTANT) and if ( variable & CONSTANT)
>     and so forth.
> 
> Further, some places had weird flag testing code like this:
> if ( (variable & CONSTANT) > 0 )
> 
> I changed them all to 
> 
> if (variable & CONSTANT)
> 
> Two things I noticed but did not fix:
> 
> A. In some places a positive error code is returned. For example
>    ENOBUF and not -ENOBUF. I assume its a bug but did not touch it.

  I'm glad you did not change this, it's not a bug. In parts of both
the send and recv data paths positive and negative returns have different
meaning. negative return is a hard error, while positive return is used
to indicate that the data path is in a flow control situation and that
the action will need to be retried later. Zero as usual means success.

> Looks like a lot of changes, I went over them several times and
> everything looks in order to me. I really hope its applied before
> any other change, it almost surely will conflict with any other
> patch, and it'll be a lot of work to re-diff.

  I found a few problem spots that I fixed up, before commiting
the code. The third problem was in an #if 0, so it wasn't an
immediate issue. Thanks.

-Libor

@@ -1302,7 +1301,7 @@ static int _sdp_inet_shutdown(struct soc
         *       1 - send shutdown
         *       2 - send/recv shutdown.
         */
-       if (0 > flag || 2 < flag)
+       if (flag < 0 || flag < 2)
                return -EINVAL;

@@ -787,7 +787,7 @@ void sdp_iocb_q_cancel(struct sdpc_iocb_
             counter < total; counter++) {
                next = iocb->next;
 
-               if (0 < (mask & iocb->flags) || SDP_IOCB_F_ALL == mask) {
+               if ((mask & iocb->flags) || msk == SDP_IOCB_F_ALL) {
                        sdp_dbg_err("IOCB <%d> cancel <%Zu> flag <%04x> "
                                    "size <%Zu:%d:%d>",
                                    iocb->key, comp, iocb->flags, iocb->size,

@@ -1092,35 +1092,35 @@ static int _sdp_inet_recv_urg(struct soc
 
        conn = SDP_GET_CONN(sk);
 
-       if (sock_flag(sk, SOCK_URGINLINE) || 0 == conn->rcv_urg_cnt)
+       if (sock_flag(sk, SOCK_URGINLINE) || !conn->rcv_urg_cnt)
                return -EINVAL;
 
        /*
         * don't cosume data on PEEK, but do consume data on TRUNC
         */
 #if 0
-       value = (0 < (MSG_PEEK & flags)) || (0 == size) ? 0 : 1;
+       value = (MSG_PEEK & flags) || size ? 1 : 0;
 #else
-       value = (0 < (MSG_PEEK & flags)) ? 0 : 1;
+       value = (MSG_PEEK & flags) ? 0 : 1;
 #endif