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

Michael S. Tsirkin mst at mellanox.co.il
Mon Feb 28 13:34:06 PST 2005


Quoting r. Libor Michalek <libor at topspin.com>:
> Subject: Re: [PATCH] sdp conditional code cleanup
> 
> 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
> 

Here is a small number of additional swaps I missed on the first pass.

Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>

Index: sdp_inet.c
===================================================================
--- sdp_inet.c	(revision 1929)
+++ sdp_inet.c	(working copy)
@@ -377,7 +377,7 @@ static int _sdp_inet_release(struct sock
 	 * Skip lingering/canceling if 
 	 * non-blocking and not exiting.
 	 */
-	if (!(MSG_DONTWAIT & flags) ||
+	if (!(flags & MSG_DONTWAIT) ||
 	    (PF_EXITING & current->flags)) {
 		/*
 		 * Wait if linger is set and
Index: sdp_send.c
===================================================================
--- sdp_send.c	(revision 1929)
+++ sdp_send.c	(working copy)
@@ -2044,7 +2044,7 @@ int sdp_inet_send(struct kiocb *req,
 	/*
 	 * set oob flag.
 	 */
-	oob = (MSG_OOB & msg->msg_flags);
+	oob = (msg->msg_flags & MSG_OOB);
 
 	sk = sock->sk;
 	conn = SDP_GET_CONN(sk);
@@ -2245,7 +2245,7 @@ done:
 	sdp_conn_unlock(conn);
 	result = ((copied > 0) ? copied : result);
 
-	if (result == -EPIPE && !(MSG_NOSIGNAL & msg->msg_flags))
+	if (result == -EPIPE && !(msg->msg_flags & MSG_NOSIGNAL))
 		send_sig(SIGPIPE, current, 0);
 
 	return result;
Index: sdp_recv.c
===================================================================
--- sdp_recv.c	(revision 1929)
+++ sdp_recv.c	(working copy)
@@ -1099,9 +1099,9 @@ static int _sdp_inet_recv_urg(struct soc
 	 * don't cosume data on PEEK, but do consume data on TRUNC
 	 */
 #if 0
-	value = (MSG_PEEK & flags) || !size ? 0 : 1;
+	value = (flags & MSG_PEEK) || !size ? 0 : 1;
 #else
-	value = (MSG_PEEK & flags) ? 0 : 1;
+	value = (flags & MSG_PEEK) ? 0 : 1;
 #endif
 
 	result = sdp_buff_q_trav_head(&conn->recv_pool,
@@ -1120,7 +1120,7 @@ static int _sdp_inet_recv_urg(struct soc
 		/*
 		 * clear urgent pointer on consumption
 		 */
-		if (!(MSG_PEEK & flags)) {
+		if (!(flags & MSG_PEEK)) {
 			conn->rcv_urg_cnt -= 1;
 			conn->byte_strm -= 1;
 			
@@ -1191,10 +1191,10 @@ int sdp_inet_recv(struct kiocb  *req,
 	/*
 	 * TODO: unhandled, but need to be handled.
 	 */
-	if (MSG_TRUNC & flags)
+	if (flags & MSG_TRUNC)
 		return -EOPNOTSUPP;
 
-	if (MSG_PEEK & flags) {
+	if (flags & MSG_PEEK) {
 		sdp_buff_q_init(&peek_queue);
 		msg->msg_flags |= MSG_PEEK;
 	}
@@ -1209,7 +1209,7 @@ int sdp_inet_recv(struct kiocb  *req,
 	/*
 	 * process urgent data
 	 */
-	if (MSG_OOB & flags) {
+	if (flags & MSG_OOB) {
 		result = _sdp_inet_recv_urg(sk, msg, size, flags);
 		copied = (result > 0) ? result : 0;
 		result = (result > 0) ? 0 : result;
@@ -1218,8 +1218,8 @@ int sdp_inet_recv(struct kiocb  *req,
 	/*
 	 * get socket values we'll need.
 	 */
-	timeout   = sock_rcvtimeo(sk, (MSG_DONTWAIT & flags));
-	low_water = sock_rcvlowat(sk, (MSG_WAITALL & flags), size);
+	timeout   = sock_rcvtimeo(sk, (flags & MSG_DONTWAIT));
+	low_water = sock_rcvlowat(sk, (flags & MSG_WAITALL), size);
 	/*
 	 * process data first, and then check condition, then wait
 	 */
@@ -1266,7 +1266,7 @@ int sdp_inet_recv(struct kiocb  *req,
 							length = 0;
 							update =
 							    (0 <
-							     (MSG_PEEK & flags))
+							     (flags & MSG_PEEK))
 							    ? 0 : 1;
 						}
 					}
@@ -1290,7 +1290,7 @@ int sdp_inet_recv(struct kiocb  *req,
 					goto done;
 				}
 #endif
-				update = (MSG_PEEK & flags) ? 0 : copy;
+				update = (flags & MSG_PEEK) ? 0 : copy;
 			}
 
 			SDP_CONN_STAT_RECV_INC(conn, update);
@@ -1314,7 +1314,7 @@ int sdp_inet_recv(struct kiocb  *req,
 				break;
 			}
 
-			if (MSG_PEEK & flags) {
+			if (flags & MSG_PEEK) {
 				expect = sdp_buff_q_put_head(&peek_queue,
 							     buff);
 				SDP_EXPECT(expect >= 0);
@@ -1514,7 +1514,7 @@ done:
 	/*
 	 * return any peeked buffers to the recv queue, in the correct order.
 	 */
-	if (MSG_PEEK & flags) {
+	if (flags & MSG_PEEK) {
 		while ((buff = sdp_buff_q_get_tail(&peek_queue))) {
 			expect = sdp_buff_q_put_head(&conn->recv_pool, buff);
 			SDP_EXPECT(expect >= 0);
Index: sdp_iocb.c
===================================================================
--- sdp_iocb.c	(revision 1929)
+++ sdp_iocb.c	(working copy)
@@ -787,7 +787,7 @@ void sdp_iocb_q_cancel(struct sdpc_iocb_
 	     counter < total; counter++) {
 		next = iocb->next;
 
-		if ((mask & iocb->flags) || mask == SDP_IOCB_F_ALL) {
+		if ((iocb->flags & mask) || mask == SDP_IOCB_F_ALL) {
 			sdp_dbg_err("IOCB <%d> cancel <%Zu> flag <%04x> "
 				    "size <%Zu:%d:%d>",
 				    iocb->key, comp, iocb->flags, iocb->size,

-- 
MST - Michael S. Tsirkin



More information about the general mailing list