| Message ID | 20170614201720.21070-1-johannes@sipsolutions.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Johannes Berg <johannes@sipsolutions.net> Date: Wed, 14 Jun 2017 22:17:20 +0200 > From: Johannes Berg <johannes.berg@intel.com> > > It's nicer to return void, since then there's no need to > cast to any structures. Currently none of the users have > a cast, but a number of future conversions do. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Applied. Although a bit disruptive, it might be nice to convert all of the other "char *" related data pointers in skbuff based interfaces. No, I'm not asking you specifically to work on this, relax :)
On Thu, 2017-06-15 at 12:18 -0400, David Miller wrote: > Although a bit disruptive, it might be nice to convert all of the > other "char *" related data pointers in skbuff based interfaces. I think it'd actually be pretty easy, since there are very few cases where you need non-void, e.g. *skb_put(skb, 1) = 'x'; Seems pretty unlikely we have that, and in any case the compiler would warn (error?) there if skb_put() becomes void. > No, I'm not asking you specifically to work on this, relax :) :-) johannes
From: Johannes Berg <johannes@sipsolutions.net> Date: Thu, 15 Jun 2017 21:28:32 +0200 > On Thu, 2017-06-15 at 12:18 -0400, David Miller wrote: > >> Although a bit disruptive, it might be nice to convert all of the >> other "char *" related data pointers in skbuff based interfaces. > > I think it'd actually be pretty easy, since there are very few cases > where you need non-void, e.g. > > *skb_put(skb, 1) = 'x'; > > Seems pretty unlikely we have that, and in any case the compiler would > warn (error?) there if skb_put() becomes void. Actually I am pretty sure I've seen a pattern like that somewhere. :-)
On Thu, 2017-06-15 at 17:26 -0400, David Miller wrote: > > > *skb_put(skb, 1) = 'x'; > > > > Seems pretty unlikely we have that, and in any case the compiler > would > > warn (error?) there if skb_put() becomes void. > > Actually I am pretty sure I've seen a pattern like that somewhere. :- > ) Yeah, there are actually a ton of them, and oddly enough my spatch is failing to catch _one_ of them?? Still refining it :) johannes
On Thu, 2017-06-15 at 23:28 +0200, Johannes Berg wrote: > On Thu, 2017-06-15 at 17:26 -0400, David Miller wrote: > > > > > *skb_put(skb, 1) = 'x'; > > > > > > Seems pretty unlikely we have that, and in any case the compiler > > > > would > > > warn (error?) there if skb_put() becomes void. > > > > Actually I am pretty sure I've seen a pattern like that somewhere. :- > > ) > > Yeah, there are actually a ton of them, and oddly enough my spatch is > failing to catch _one_ of them?? Still refining it :) I suggest changing those to skb_put_char(skb, char) in a first pass and then doing the other bits later. Here's a script that does the conversion. $ /usr/bin/git grep -P --name-only "\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);" | \ xargs perl -p -i -e 's/\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);/skb_put_char(\1, \2);/'
On Thu, 2017-06-15 at 15:17 -0700, Joe Perches wrote: > I suggest changing those to skb_put_char(skb, char) That might be something to think of, but you can't really know for sure that they're not using len > 1 and don't yet care about the other bytes or something. That'd probably be another bug, but ... dunno And anyway, I think *(u8 *)skb_put(skb, 1) = c; isn't really that bad. Obviously that could be converted further to skb_put_char(), using a simple spatch: @@ expression SKB, C; @@ - *(u8 *)skb_put(SKB, 1) = C; + skb_put_char(SKB, C); > Here's a script that does the conversion. > > $ /usr/bin/git grep -P --name-only > "\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);" | > \ > xargs perl -p -i -e 's/\*\s*skb_put\s*\(\s*([\w\.\[\]\>\- > ]+)\s*,\s*1\s*\)\s*=\s*([^;]+);/skb_put_char(\1, \2);/' Uh, I think you're using the wrong tool for the job :-) johannes
On Thu, 2017-06-15 at 15:17 -0700, Joe Perches wrote: > Here's a script that does the conversion. > > $ /usr/bin/git grep -P --name-only > "\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);" | > \ > xargs perl -p -i -e 's/\*\s*skb_put\s*\(\s*([\w\.\[\]\>\- > ]+)\s*,\s*1\s*\)\s*=\s*([^;]+);/skb_put_char(\1, \2);/' Btw, this is incomplete - you have "\*\s*" at the beginning, but there are cases like *(skb_put(skb, 1)) = c; where you have extra parentheses. By just adding them to the spatch, it finds both cases trivially. I'm much more comfortable using spatch to do things like this. johannes
On Fri, 2017-06-16 at 00:21 +0200, Johannes Berg wrote: > On Thu, 2017-06-15 at 15:17 -0700, Joe Perches wrote: > > > I suggest changing those to skb_put_char(skb, char) > > That might be something to think of, but you can't really know for sure > that they're not using len > 1 and don't yet care about the other bytes > or something. That'd probably be another bug, but ... dunno > > And anyway, I think > > *(u8 *)skb_put(skb, 1) = c; > > isn't really that bad. Obviously that could be converted further to > skb_put_char(), using a simple spatch: > > @@ > expression SKB, C; > @@ > - *(u8 *)skb_put(SKB, 1) = C; > + skb_put_char(SKB, C); > > > > Here's a script that does the conversion. > > > > $ /usr/bin/git grep -P --name-only > > "\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);" | > > \ > > xargs perl -p -i -e 's/\*\s*skb_put\s*\(\s*([\w\.\[\]\>\- > > ]+)\s*,\s*1\s*\)\s*=\s*([^;]+);/skb_put_char(\1, \2);/' > > Uh, I think you're using the wrong tool for the job :-) I'm familiar with both. It depends on how much you want to wait. The thing I wrote finished in about 2 seconds on my little laptop. cheers, Joe
On Fri, 2017-06-16 at 00:23 +0200, Johannes Berg wrote: > On Thu, 2017-06-15 at 15:17 -0700, Joe Perches wrote: > > > Here's a script that does the conversion. > > > > $ /usr/bin/git grep -P --name-only > > "\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);" | > > \ > > xargs perl -p -i -e 's/\*\s*skb_put\s*\(\s*([\w\.\[\]\>\- > > ]+)\s*,\s*1\s*\)\s*=\s*([^;]+);/skb_put_char(\1, \2);/' > > Btw, this is incomplete - you have "\*\s*" at the beginning, but there > are cases like > > *(skb_put(skb, 1)) = c; > > where you have extra parentheses. By just adding them to the spatch, it > finds both cases trivially. > > I'm much more comfortable using spatch to do things like this. Knock your self out. Whatever floats your boat. Have at it. Go get 'em. etc... There are also some uses like: memcpy(skb_put(h5->rx_skb, 1), byte, 1); that could also be converted. cheers, Joe
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 1151b50892d1..01ea64d0783a 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1904,9 +1904,9 @@ static inline unsigned char *__skb_put(struct sk_buff *skb, unsigned int len) return tmp; } -static inline unsigned char *skb_put_zero(struct sk_buff *skb, unsigned int len) +static inline void *skb_put_zero(struct sk_buff *skb, unsigned int len) { - unsigned char *tmp = skb_put(skb, len); + void *tmp = skb_put(skb, len); memset(tmp, 0, len);