[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