| Message ID | 4B06AC2C.3070102@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: William Allen Simpson <william.allen.simpson@gmail.com> Date: Fri, 20 Nov 2009 09:48:12 -0500 > + if (ctd.tcpct_used > sizeof(ctd.tcpct_value) > + || ctd.tcpct_s_data_desired > TCP_MSS_DESIRED) > + return -EINVAL; Please fix this conditional coding style. > + } else if ((0x1 & ctd.tcpct_cookie_desired) > + || ctd.tcpct_cookie_desired > TCP_COOKIE_MAX > + || ctd.tcpct_cookie_desired < TCP_COOKIE_MIN) { > + return -EINVAL; Same here. > + if (ctd.tcpct_used > 0 > + || (tp->cookie_values == NULL > + && (sysctl_tcp_cookie_size > 0 > + || ctd.tcpct_cookie_desired > 0 > + || ctd.tcpct_s_data_desired > 0))) { Please fix the conditional coding style, and the alignment of the lines, it's not right here. > + ctd.tcpct_flags = (tp->rx_opt.cookie_in_always > + ? TCP_COOKIE_IN_ALWAYS : 0) > + | (tp->rx_opt.cookie_out_never > + ? TCP_COOKIE_OUT_NEVER : 0); "?" should be at end of previous line not at beginning of next one, please fix this up. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2009-11-20 at 09:26 -0800, David Miller wrote: > From: William Allen Simpson <william.allen.simpson@gmail.com> > > + ctd.tcpct_flags = (tp->rx_opt.cookie_in_always > > + ? TCP_COOKIE_IN_ALWAYS : 0) > > + | (tp->rx_opt.cookie_out_never > > + ? TCP_COOKIE_OUT_NEVER : 0); > > "?" should be at end of previous line not at beginning of > next one, please fix this up. Perhaps the ? position is taster's choice. (false positives exist) $ grep -rP --include=*.[ch] "^\s*\?" * | wc -l 884 $ grep -rP --include=*.[ch] -h "\?\s*$" * | \ grep -vP "^\s*\*" | grep -vP "(//|/\*)" | wc -l 2968 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: William Allen Simpson <william.allen.simpson@gmail.com> > Date: Fri, 20 Nov 2009 09:48:12 -0500 >> + if (ctd.tcpct_used > 0 >> + || (tp->cookie_values == NULL >> + && (sysctl_tcp_cookie_size > 0 >> + || ctd.tcpct_cookie_desired > 0 >> + || ctd.tcpct_s_data_desired > 0))) { > > Please fix the conditional coding style, and the alignment of > the lines, it's not right here. > Eliding the repeated assertions, and focusing on this example. This is not addressed (nor forbidden) in CodingStyle. As Joe noted earlier with '?' and ':', there are ample examples throughout the code base of this style, including here and there among the tcp*.c files. Obviously, this is very easy to read! Coding style is all about readability and maintainability using commonly available tools. However, grep shows that the "||" or "&&" is elsewhere most often indented 4 for each level (although there is some inconsistency). Either 1, 2, or 4 distinguishes <condition> indentation from <statements> indented by tab (8). In my experience, I've found 1 best, as that lines up variables and parentheses levels, and it's easy to type. But it certainly would be easy enough to indent by 2 or 4 instead. BTW, the 'indent' program turns this into incomprehensible and unmaintainable garbage. Of course, there are ample examples of garbage in these tcp*.c files, too.... :-( Therefore, David, would indentation of 2 or 4 be preferable here? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2009-11-22 at 01:25 -0500, William Allen Simpson wrote: > David Miller wrote: > > From: William Allen Simpson <william.allen.simpson@gmail.com> > > Date: Fri, 20 Nov 2009 09:48:12 -0500 > >> + if (ctd.tcpct_used > 0 > >> + || (tp->cookie_values == NULL > >> + && (sysctl_tcp_cookie_size > 0 > >> + || ctd.tcpct_cookie_desired > 0 > >> + || ctd.tcpct_s_data_desired > 0))) { > > Please fix the conditional coding style, and the alignment of > > the lines, it's not right here. > David, would indentation of 2 or 4 be preferable here? Hi William. I think the rather significantly majority style, especially for net/... is to use || and && at the end of the line rather than the start and it should be used. Treewide: $ grep -rP --include=*.[ch] "(\|\||\&\&)\s*$" * | wc -l 34180 $ grep -rP --include=*.[ch] "^\s*(\|\||\&\&)" * | wc -l 7855 net: 3859 to 382 (more than 10:1, so it's the one to follow) drivers/net: 4610 to 666 Besides, it's the one David wants... cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: William Allen Simpson <william.allen.simpson@gmail.com> Date: Sun, 22 Nov 2009 01:25:43 -0500 > Therefore, David, would indentation of 2 or 4 be preferable here? It should be of the form: if (x && y) or: if (x && y) Fix patches, rather than complaints, for existing cases where things do not follow this pattern are cetainly welcome. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Joe Perches wrote: > On Sun, 2009-11-22 at 01:25 -0500, William Allen Simpson wrote: >> David Miller wrote: >>> From: William Allen Simpson <william.allen.simpson@gmail.com> >>> Date: Fri, 20 Nov 2009 09:48:12 -0500 >>>> + if (ctd.tcpct_used > 0 >>>> + || (tp->cookie_values == NULL >>>> + && (sysctl_tcp_cookie_size > 0 >>>> + || ctd.tcpct_cookie_desired > 0 >>>> + || ctd.tcpct_s_data_desired > 0))) { >>> Please fix the conditional coding style, and the alignment of >>> the lines, it's not right here. > > I think the rather significantly majority style, especially > for net/... is to use || and && at the end of the line rather > than the start and it should be used. > > Treewide: > > $ grep -rP --include=*.[ch] "(\|\||\&\&)\s*$" * | wc -l > 34180 > > $ grep -rP --include=*.[ch] "^\s*(\|\||\&\&)" * | wc -l > 7855 > > net: 3859 to 382 (more than 10:1, so it's the one to follow) > drivers/net: 4610 to 666 > Thanks for measuring. I'll note that during the previous review back at the v4 round, you (Joe) passed along a formerly private message from Linus expressing his preference for variable lvalues: http://www.spinics.net/lists/netdev/msg111212.html But my example code in that thread also had both leading && and || -- and neither David nor Eric nor Ilpo nor you mentioned that as an issue in that entire thread: http://www.spinics.net/lists/netdev/msg111172.html In the previous plaint about lvalues, there were merely 500+ examples using the same constant lvalue form as my code -- in arch, drivers, net, and sound. For this example, many *thousands* are found everywhere! Therefore, it's plain as can be that this is just more jumping through arbitrary and capricious hoops that others are not required to follow. > Besides, it's the one David wants... > At *thousands* of examples, including in the tcp*.c files themselves, it really becomes obvious that that may be a personal preference of David, but is *not* a tree-wide or even a net-wide coding style. However, a private message to me nearly 2 months ago expressed: "As unpalatable as it may be, all the more reason to genuflect as required to get the changes into the net-next-2.6 tree so they will flow down to future distros." I followed that advice for a month. That last patch submitted for inclusion was v4 on Oct 27th. Then, as some have noticed, I quit using the net-next tree for actual development. I've only sent weekly RFC versions to solicit more widespread comments from subject matter experts, and keep the patch offsets in sync with the rapidly changing tree. As a more recent private comment asked: "So your frustration is nothing but normal. And guess what ? Few people accept the challenge, so keep trying !" So, I'll try again now, with the assurance that this is the final hoop. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2009-11-23 at 06:16 -0500, William Allen Simpson wrote: > Therefore, it's plain as can be that this is just more jumping through > arbitrary and capricious hoops that others are not required to follow. > At *thousands* of examples, including in the tcp*.c files themselves, it > really becomes obvious that that may be a personal preference of David, > but is *not* a tree-wide or even a net-wide coding style. It seems similar kernel source uses are mostly historic. Relatively little new code is added with leading || or &&. There is some in staging/, but it normally gets removed. > So, I'll try again now, with the assurance that this is the final hoop. Once more into the breach or men of few words are the best men? I'll stop typing now... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: William Allen Simpson <william.allen.simpson@gmail.com> Date: Mon, 23 Nov 2009 06:16:03 -0500 > Therefore, it's plain as can be that this is just more jumping through > arbitrary and capricious hoops that others are not required to follow. Others, like Eric and Ilpo, don't have to jump through these hoops because when they submit patches their code doesn't have coding style issues like your's does. Trust me, it's not about you, and you're not being picked out specifically. These things would be enforced upon anyone. The only unique aspect about you is that you seem to respond to these requirements in the most sour manner possible rather than pleasantly complying and thus getting your changes added to the tree. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 1356e3d..5b14e2b 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -2084,8 +2084,9 @@ static int do_tcp_setsockopt(struct sock *sk, int level, int val; int err = 0; - /* This is a string value all the others are int's */ - if (optname == TCP_CONGESTION) { + /* These are data/string values, all the others are ints */ + switch (optname) { + case TCP_CONGESTION: { char name[TCP_CA_NAME_MAX]; if (optlen < 1) @@ -2102,6 +2103,93 @@ static int do_tcp_setsockopt(struct sock *sk, int level, release_sock(sk); return err; } + case TCP_COOKIE_TRANSACTIONS: { + struct tcp_cookie_transactions ctd; + struct tcp_cookie_values *cvp = NULL; + + if (sizeof(ctd) > optlen) + return -EINVAL; + if (copy_from_user(&ctd, optval, sizeof(ctd))) + return -EFAULT; + + if (ctd.tcpct_used > sizeof(ctd.tcpct_value) + || ctd.tcpct_s_data_desired > TCP_MSS_DESIRED) + return -EINVAL; + + if (ctd.tcpct_cookie_desired == 0) { + /* default to global value */ + } else if ((0x1 & ctd.tcpct_cookie_desired) + || ctd.tcpct_cookie_desired > TCP_COOKIE_MAX + || ctd.tcpct_cookie_desired < TCP_COOKIE_MIN) { + return -EINVAL; + } + + if (TCP_COOKIE_OUT_NEVER & ctd.tcpct_flags) { + /* Supercedes all other values */ + lock_sock(sk); + if (tp->cookie_values != NULL) { + kref_put(&tp->cookie_values->kref, + tcp_cookie_values_release); + tp->cookie_values = NULL; + } + tp->rx_opt.cookie_in_always = 0; /* false */ + tp->rx_opt.cookie_out_never = 1; /* true */ + release_sock(sk); + return err; + } + + /* Allocate ancillary memory before locking. + */ + if (ctd.tcpct_used > 0 + || (tp->cookie_values == NULL + && (sysctl_tcp_cookie_size > 0 + || ctd.tcpct_cookie_desired > 0 + || ctd.tcpct_s_data_desired > 0))) { + cvp = kzalloc(sizeof(*cvp) + ctd.tcpct_used, + GFP_KERNEL); + if (cvp == NULL) + return -ENOMEM; + } + lock_sock(sk); + tp->rx_opt.cookie_in_always = + (TCP_COOKIE_IN_ALWAYS & ctd.tcpct_flags); + tp->rx_opt.cookie_out_never = 0; /* false */ + + if (tp->cookie_values != NULL) { + if (cvp != NULL) { + /* Changed values are recorded by a changed + * pointer, ensuring the cookie will differ, + * without separately hashing each value later. + */ + kref_put(&tp->cookie_values->kref, + tcp_cookie_values_release); + kref_init(&cvp->kref); + tp->cookie_values = cvp; + } else { + cvp = tp->cookie_values; + } + } + if (cvp != NULL) { + cvp->cookie_desired = ctd.tcpct_cookie_desired; + + if (ctd.tcpct_used > 0) { + memcpy(cvp->s_data_payload, ctd.tcpct_value, + ctd.tcpct_used); + cvp->s_data_desired = ctd.tcpct_used; + cvp->s_data_constant = 1; /* true */ + } else { + /* No constant payload data. */ + cvp->s_data_desired = ctd.tcpct_s_data_desired; + cvp->s_data_constant = 0; /* false */ + } + } + release_sock(sk); + return err; + } + default: + /* fallthru */ + break; + }; if (optlen < sizeof(int)) return -EINVAL; @@ -2426,6 +2514,47 @@ static int do_tcp_getsockopt(struct sock *sk, int level, if (copy_to_user(optval, icsk->icsk_ca_ops->name, len)) return -EFAULT; return 0; + + case TCP_COOKIE_TRANSACTIONS: { + struct tcp_cookie_transactions ctd; + struct tcp_cookie_values *cvp = tp->cookie_values; + + if (get_user(len, optlen)) + return -EFAULT; + if (len < sizeof(ctd)) + return -EINVAL; + + memset(&ctd, 0, sizeof(ctd)); + ctd.tcpct_flags = (tp->rx_opt.cookie_in_always + ? TCP_COOKIE_IN_ALWAYS : 0) + | (tp->rx_opt.cookie_out_never + ? TCP_COOKIE_OUT_NEVER : 0); + + if (cvp != NULL) { + ctd.tcpct_flags |= (cvp->s_data_in + ? TCP_S_DATA_IN : 0) + | (cvp->s_data_out + ? TCP_S_DATA_OUT : 0); + + ctd.tcpct_cookie_desired = cvp->cookie_desired; + ctd.tcpct_s_data_desired = cvp->s_data_desired; + + /* Cookie(s) saved, return as nonce */ + if (sizeof(ctd.tcpct_value) < cvp->cookie_pair_size) { + /* impossible? */ + return -EINVAL; + } + memcpy(&ctd.tcpct_value[0], &cvp->cookie_pair[0], + cvp->cookie_pair_size); + ctd.tcpct_used = cvp->cookie_pair_size; + } + + if (put_user(sizeof(ctd), optlen)) + return -EFAULT; + if (copy_to_user(optval, &ctd, sizeof(ctd))) + return -EFAULT; + return 0; + } default: return -ENOPROTOOPT; }
Provide per socket control of the TCP cookie option and SYN/SYNACK data. This is a straightforward re-implementation of an earlier (year-old) patch that no longer applies cleanly, with permission of the original author (Adam Langley): http://thread.gmane.org/gmane.linux.network/102586 The principle difference is using a TCP option to carry the cookie nonce, instead of a user configured offset in the data. Allocations have been rearranged to avoid requiring GFP_ATOMIC. Requires: net: TCP_MSS_DEFAULT, TCP_MSS_DESIRED TCPCT part 1c: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS TCPCT part 1d: define TCP cookie option, extend existing struct's Signed-off-by: William.Allen.Simpson@gmail.com --- net/ipv4/tcp.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 131 insertions(+), 2 deletions(-)