| Message ID | 4B450065.4010108@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le 06/01/2010 22:28, William Allen Simpson a écrit : > Redefine two TCP header functions to accept TCP header pointer. > When subtracting, return signed int to allow error checking. > > These functions will be used in subsequent patches that implement > additional features. > > Signed-off-by: William.Allen.Simpson@gmail.com > --- > include/linux/tcp.h | 12 ++++++++++++ > 1 files changed, 12 insertions(+), 0 deletions(-) > Its better to inline your patches so that we can comment them, without copy/paste When I hit 'reply to', my mailer only quoted the ChangeLog, not the patch. Anyway .. +/* Length of standard options only. This could be negative. */ +static inline int tcp_option_len_th(const struct tcphdr *th) +{ + return (int)(th->doff * 4) - sizeof(*th); +} The (int) cast is not necessary, since the function returns a signed int -> return th->doff * 4 - sizeof(*th); -- 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
Eric Dumazet wrote: > Its better to inline your patches so that we can comment them, without copy/paste > > When I hit 'reply to', my mailer only quoted the ChangeLog, not the patch. > Seeing that we're both using Mozilla, how to you do it? It took me many attempts to get this to work with Thunderbird on the Mac. > Anyway .. > > +/* Length of standard options only. This could be negative. */ > +static inline int tcp_option_len_th(const struct tcphdr *th) > +{ > + return (int)(th->doff * 4) - sizeof(*th); > +} > > > The (int) cast is not necessary, since the function returns a signed int > > -> > return th->doff * 4 - sizeof(*th); > Then GCC must be smarter than it was in the past, as doff is an __u16 bit slice -- once upon a time, a cast was required before subtraction. One of the dis/advantages of C programming for 30+ years is that my fingers remember some fairly old practices.... (But this still works properly!) -- 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
Le 12/01/2010 18:42, William Allen Simpson a écrit : > Eric Dumazet wrote: >> Its better to inline your patches so that we can comment them, without >> copy/paste >> >> When I hit 'reply to', my mailer only quoted the ChangeLog, not the >> patch. >> > Seeing that we're both using Mozilla, how to you do it? > > It took me many attempts to get this to work with Thunderbird on the Mac. > Hmm, I followed Documentation/email-clients.txt tricks, I dont remember exact details. I type my Changelog text, add my signature, then copy/paste patch from external editor (this editor must preserve tabulations of course) Its probably a bit odd, because I am stuck with a windows XP notebook here at work, dont flame me :( About cast games, maybe following way is the cleanest one. int tcp_options_len_th(struct tcphdr *th) { return tcp_header_len_th(th) - sizeof(*th); } -- 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
Eric Dumazet wrote, On 01/12/2010 06:53 PM: > Its probably a bit odd, because I am stuck with a windows XP notebook here at work, > dont flame me :( You should be ashamed! (Linus promoted Windows 7 long time ago ;-) Jarek P. -- 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
Eric Dumazet wrote: > I type my Changelog text, add my signature, then copy/paste patch from external editor > (this editor must preserve tabulations of course) > That doesn't work properly on a Mac, copying from BBEdit to Thunderbird. BBEdit preserves tabs and even understands and preserves Unix LF (and I've been using it for a Unix editor since it was included with Xinu in early '90s), but the MacOS copy and paste seems to mangle it. I'll try again someday with Thunderbird 3, when it's had time to mature. > About cast games, maybe following way is the cleanest one. > > int tcp_options_len_th(struct tcphdr *th) > { > return tcp_header_len_th(th) - sizeof(*th); > } > If you'd have been one of my C students, you'd have failed the exam question. That's unsigned int tcp_header_len_th() -- subtracting an untyped constant could be a negative number (stored in an unsigned). Then demotion to int (which many compilers truncate to a very large positive number). It's one of the reasons that folks used to do all this with macros, so that the types and truncation were handled well by the compiler. Of course, this is an inline function, which is more like macros. I've not studied how gcc works internally since egcs. Let's keep (int)(th->doff * 4) - sizeof(*th) -- self documenting, and should work with a wide variety of compilers. -- 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
Le 13/01/2010 09:53, William Allen Simpson a écrit : > Eric Dumazet wrote: > >> About cast games, maybe following way is the cleanest one. >> >> int tcp_options_len_th(struct tcphdr *th) >> { >> return tcp_header_len_th(th) - sizeof(*th); >> } >> > If you'd have been one of my C students, you'd have failed the exam > question. That's unsigned int tcp_header_len_th() -- subtracting an > untyped constant could be a negative number (stored in an unsigned). > Then demotion to int (which many compilers truncate to a very large > positive number). Thats simply not true, you are very confused. Once again you type too much text and ignore my comments. I really would hate being your student, thanks God it wont happen in this life. 1) You wrote tcp_header_len_th(), you should know that it returns an unsigned int. 2) You also should know that sizeof() is *strongly* typed (size_t), not an "untyped constant". unsigned int arg = some_expression; size_t sz = sizeof(something) int res = arg - sz; return res; is *perfectly* legal and very well defined by C standards. It *will* return a negative value is arg < sz > > It's one of the reasons that folks used to do all this with macros, so > that the types and truncation were handled well by the compiler. > > Of course, this is an inline function, which is more like macros. I've > not studied how gcc works internally since egcs. > > Let's keep (int)(th->doff * 4) - sizeof(*th) -- self documenting, and > should work with a wide variety of compilers. So you wrote tcp_header_len_th(), but you keep (th->doff * 4) thing all over the code... The (int) cast it not only _not_ needed, its also confusing. -- 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
Eric Dumazet wrote: 2) You also should know that sizeof() is *strongly* typed (size_t), > not an "untyped constant". > My apologies, it's fairly early in the morning here -- I meant "unsigned" rather than "untyped". > The (int) cast it not only _not_ needed, its also confusing. > I'm sorry for your confusion. I believe it adds clarity. Moreover, it's fairly egregious that the old tcp_hdrlen() contributor didn't take signed versus unsigned into account. Perhaps we could move along to more substantive issues.... Have you had an opportunity to test PATCH 2/2 in this series? -- 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/include/linux/tcp.h b/include/linux/tcp.h index 7fee8a4..d0133cf 100644 --- a/include/linux/tcp.h +++ b/include/linux/tcp.h @@ -223,6 +223,18 @@ static inline unsigned int tcp_optlen(const struct sk_buff *skb) return (tcp_hdr(skb)->doff - 5) * 4; } +/* Length of fixed header plus standard options. */ +static inline unsigned int tcp_header_len_th(const struct tcphdr *th) +{ + return th->doff * 4; +} + +/* Length of standard options only. This could be negative. */ +static inline int tcp_option_len_th(const struct tcphdr *th) +{ + return (int)(th->doff * 4) - sizeof(*th); +} + /* This defines a selective acknowledgement block. */ struct tcp_sack_block_wire { __be32 start_seq;
Redefine two TCP header functions to accept TCP header pointer. When subtracting, return signed int to allow error checking. These functions will be used in subsequent patches that implement additional features. Signed-off-by: William.Allen.Simpson@gmail.com --- include/linux/tcp.h | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)