* [PATCH] tipc: set sk_err correctly when connection fails @ 2013-08-23 13:56 erik.hugne 2013-08-27 0:45 ` Ying Xue 2013-08-27 13:20 ` Paul Gortmaker 0 siblings, 2 replies; 6+ messages in thread From: erik.hugne @ 2013-08-23 13:56 UTC (permalink / raw) To: netdev, jon.maloy, paul.gortmaker; +Cc: nhan.tt.vo, tipc-discussion From: Erik Hugne <erik.hugne@ericsson.com> This fixes a problem when connect() fails and returns the error code as a positive value, whereas errno itself is never set. The reason is that error codes set in sk_err should never be inverted. Signed-off-by: Erik Hugne <erik.hugne@ericsson.com> --- net/tipc/socket.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/tipc/socket.c b/net/tipc/socket.c index ce8249c..6cc7ddd 100644 --- a/net/tipc/socket.c +++ b/net/tipc/socket.c @@ -1257,7 +1257,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) /* Accept only ACK or NACK message */ if (unlikely(msg_errcode(msg))) { sock->state = SS_DISCONNECTING; - sk->sk_err = -ECONNREFUSED; + sk->sk_err = ECONNREFUSED; retval = TIPC_OK; break; } @@ -1268,7 +1268,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) res = auto_connect(sock, msg); if (res) { sock->state = SS_DISCONNECTING; - sk->sk_err = res; + sk->sk_err = -res; retval = TIPC_OK; break; } -- 1.7.9.5 ------------------------------------------------------------------------------ Introducing Performance Central, a new site from SourceForge and AppDynamics. Performance Central is your source for news, insights, analysis and resources for efficient Application Performance Management. Visit us today! http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tipc: set sk_err correctly when connection fails 2013-08-23 13:56 [PATCH] tipc: set sk_err correctly when connection fails erik.hugne @ 2013-08-27 0:45 ` Ying Xue 2013-08-27 13:20 ` Paul Gortmaker 1 sibling, 0 replies; 6+ messages in thread From: Ying Xue @ 2013-08-27 0:45 UTC (permalink / raw) To: erik.hugne; +Cc: netdev, jon.maloy, paul.gortmaker, tipc-discussion, nhan.tt.vo On 08/23/2013 09:56 PM, erik.hugne@ericsson.com wrote: > From: Erik Hugne <erik.hugne@ericsson.com> > > This fixes a problem when connect() fails and returns the error > code as a positive value, whereas errno itself is never set. The > reason is that error codes set in sk_err should never be inverted. > > Signed-off-by: Erik Hugne <erik.hugne@ericsson.com> Acked-by: Ying Xue <ying.xue@windriver.com> > --- > net/tipc/socket.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/tipc/socket.c b/net/tipc/socket.c > index ce8249c..6cc7ddd 100644 > --- a/net/tipc/socket.c > +++ b/net/tipc/socket.c > @@ -1257,7 +1257,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) > /* Accept only ACK or NACK message */ > if (unlikely(msg_errcode(msg))) { > sock->state = SS_DISCONNECTING; > - sk->sk_err = -ECONNREFUSED; > + sk->sk_err = ECONNREFUSED; > retval = TIPC_OK; > break; > } > @@ -1268,7 +1268,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) > res = auto_connect(sock, msg); > if (res) { > sock->state = SS_DISCONNECTING; > - sk->sk_err = res; > + sk->sk_err = -res; > retval = TIPC_OK; > break; > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tipc: set sk_err correctly when connection fails 2013-08-23 13:56 [PATCH] tipc: set sk_err correctly when connection fails erik.hugne 2013-08-27 0:45 ` Ying Xue @ 2013-08-27 13:20 ` Paul Gortmaker 2013-08-27 15:18 ` Erik Hugne 1 sibling, 1 reply; 6+ messages in thread From: Paul Gortmaker @ 2013-08-27 13:20 UTC (permalink / raw) To: erik.hugne; +Cc: jon.maloy, netdev, nhan.tt.vo, tipc-discussion On 13-08-23 09:56 AM, erik.hugne@ericsson.com wrote: > From: Erik Hugne <erik.hugne@ericsson.com> > > This fixes a problem when connect() fails and returns the error > code as a positive value, whereas errno itself is never set. The > reason is that error codes set in sk_err should never be inverted. That seems consistent with the other instances at least. ~/git/linux-head$ git grep ECONNREFUSED net|grep sk_err net/caif/caif_socket.c: cf_sk->sk.sk_err = ECONNREFUSED; net/decnet/dn_nsp_in.c: sk->sk_err = ECONNREFUSED; net/ipv4/tcp_input.c: sk->sk_err = ECONNREFUSED; net/tipc/socket.c: sk->sk_err = -ECONNREFUSED; ~/git/linux-head$ What was the high level user visible symptom in this case? Stalled connections or ... ? Thanks, Paul. -- > > Signed-off-by: Erik Hugne <erik.hugne@ericsson.com> > --- > net/tipc/socket.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/tipc/socket.c b/net/tipc/socket.c > index ce8249c..6cc7ddd 100644 > --- a/net/tipc/socket.c > +++ b/net/tipc/socket.c > @@ -1257,7 +1257,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) > /* Accept only ACK or NACK message */ > if (unlikely(msg_errcode(msg))) { > sock->state = SS_DISCONNECTING; > - sk->sk_err = -ECONNREFUSED; > + sk->sk_err = ECONNREFUSED; > retval = TIPC_OK; > break; > } > @@ -1268,7 +1268,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) > res = auto_connect(sock, msg); > if (res) { > sock->state = SS_DISCONNECTING; > - sk->sk_err = res; > + sk->sk_err = -res; > retval = TIPC_OK; > break; > } > ------------------------------------------------------------------------------ Introducing Performance Central, a new site from SourceForge and AppDynamics. Performance Central is your source for news, insights, analysis and resources for efficient Application Performance Management. Visit us today! http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tipc: set sk_err correctly when connection fails 2013-08-27 13:20 ` Paul Gortmaker @ 2013-08-27 15:18 ` Erik Hugne 2013-08-27 18:12 ` Paul Gortmaker 0 siblings, 1 reply; 6+ messages in thread From: Erik Hugne @ 2013-08-27 15:18 UTC (permalink / raw) To: Paul Gortmaker; +Cc: netdev, jon.maloy, ying.xue, tipc-discussion, nhan.tt.vo On Tue, Aug 27, 2013 at 09:20:23AM -0400, Paul Gortmaker wrote: > What was the high level user visible symptom in this case? > Stalled connections or ... ? Should the connect fail, if the publication/server is unavailable or some other error. The connect() call returns the error code directly (as a positive value). [...] socket(0x1e /* PF_??? */, SOCK_SEQPACKET, 0) = 3 setsockopt(3, 0x10f /* SOL_?? */, 129, [0], 4) = 0 setsockopt(3, 0x10f /* SOL_?? */, 127, [0], 4) = 0 connect(3, {sa_family=0x1e /* AF_??? */, sa_data="\2\1\322\4\0\0\322\4\0\0\0\0\0\0"}, 16) = 111 sendto(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 66000, 0, NULL, 0) = -1 EPIPE (Broken pipe) In the strace above, error checking was done as: if (connect(fd,.....) < 0) perror("connect"); //E ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tipc: set sk_err correctly when connection fails 2013-08-27 15:18 ` Erik Hugne @ 2013-08-27 18:12 ` Paul Gortmaker 2013-08-28 1:32 ` Ying Xue 0 siblings, 1 reply; 6+ messages in thread From: Paul Gortmaker @ 2013-08-27 18:12 UTC (permalink / raw) To: Erik Hugne; +Cc: netdev, jon.maloy, ying.xue, tipc-discussion, nhan.tt.vo On 13-08-27 11:18 AM, Erik Hugne wrote: > On Tue, Aug 27, 2013 at 09:20:23AM -0400, Paul Gortmaker wrote: >> What was the high level user visible symptom in this case? >> Stalled connections or ... ? > > Should the connect fail, if the publication/server is unavailable or some other > error. The connect() call returns the error code directly (as a positive value). Please send a v2 with the end-user visible symptom clearly described; as this information is what people use in order to triage whether commits belong in stable, or net vs. net-next etc. For example: Should the connect fail, say if the publication/server is unavailable or some other error, then the code returns a positive return value. Since most code only checks for a negative return on connect(), it tries to continue, but will ultimately fail on the 1st sendto() as the strace snippet below shows. I've said "most code" since I simply don't know what it was that you were tracing below. It would help if we knew if this part of a common application or similar. Thanks, Paul. -- > > [...] > socket(0x1e /* PF_??? */, SOCK_SEQPACKET, 0) = 3 > setsockopt(3, 0x10f /* SOL_?? */, 129, [0], 4) = 0 > setsockopt(3, 0x10f /* SOL_?? */, 127, [0], 4) = 0 > connect(3, {sa_family=0x1e /* AF_??? */, sa_data="\2\1\322\4\0\0\322\4\0\0\0\0\0\0"}, 16) = 111 > sendto(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 66000, 0, NULL, 0) = -1 EPIPE (Broken pipe) > > In the strace above, error checking was done as: > if (connect(fd,.....) < 0) > perror("connect"); > > //E > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tipc: set sk_err correctly when connection fails 2013-08-27 18:12 ` Paul Gortmaker @ 2013-08-28 1:32 ` Ying Xue 0 siblings, 0 replies; 6+ messages in thread From: Ying Xue @ 2013-08-28 1:32 UTC (permalink / raw) To: Paul Gortmaker; +Cc: Erik Hugne, netdev, jon.maloy, tipc-discussion, nhan.tt.vo On 08/28/2013 02:12 AM, Paul Gortmaker wrote: > On 13-08-27 11:18 AM, Erik Hugne wrote: >> On Tue, Aug 27, 2013 at 09:20:23AM -0400, Paul Gortmaker wrote: >>> What was the high level user visible symptom in this case? >>> Stalled connections or ... ? >> >> Should the connect fail, if the publication/server is unavailable or some other >> error. The connect() call returns the error code directly (as a positive value). > > Please send a v2 with the end-user visible symptom clearly described; > as this information is what people use in order to triage whether > commits belong in stable, or net vs. net-next etc. For example: > > Should the connect fail, say if the publication/server is unavailable or > some other error, then the code returns a positive return value. Since > most code only checks for a negative return on connect(), it tries to > continue, but will ultimately fail on the 1st sendto() as the strace > snippet below shows. > > I've said "most code" since I simply don't know what it was that you were > tracing below. It would help if we knew if this part of a common application > or similar. > Firstly it's absolutely wrong that connect() returns a positive error code - 111(i.e, ECONNREFUSED) if connect() is failed, that is, it violates POSIX standard. As the patch's changelog describes, sk_err is incorrectly set to a negative value instead of positive value in filter_connect(). In connect(), it will set its return value with sock_error(sk) if it fails. As the return value of sock_error(sk) almost can be deemed as -sk_err, this is why we see a positive error code of connect() in below strace log. But, the patch is absolutely able to fix the issue. Regards, Ying > Thanks, > Paul. > -- > >> >> [...] >> socket(0x1e /* PF_??? */, SOCK_SEQPACKET, 0) = 3 >> setsockopt(3, 0x10f /* SOL_?? */, 129, [0], 4) = 0 >> setsockopt(3, 0x10f /* SOL_?? */, 127, [0], 4) = 0 >> connect(3, {sa_family=0x1e /* AF_??? */, sa_data="\2\1\322\4\0\0\322\4\0\0\0\0\0\0"}, 16) = 111 >> sendto(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 66000, 0, NULL, 0) = -1 EPIPE (Broken pipe) >> >> In the strace above, error checking was done as: >> if (connect(fd,.....) < 0) >> perror("connect"); >> >> //E >> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-28 1:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-23 13:56 [PATCH] tipc: set sk_err correctly when connection fails erik.hugne 2013-08-27 0:45 ` Ying Xue 2013-08-27 13:20 ` Paul Gortmaker 2013-08-27 15:18 ` Erik Hugne 2013-08-27 18:12 ` Paul Gortmaker 2013-08-28 1:32 ` Ying Xue
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).