| Message ID | 1465322550.25087.40.camel@perches.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jun 07, 2016 at 11:02:30AM -0700, Joe Perches wrote: > On Tue, 2016-06-07 at 19:34 +0200, Pablo Neira Ayuso wrote: > > On Tue, Jun 07, 2016 at 10:04:40AM -0700, Joe Perches wrote: > > > One more question, is this chunk below correct from > > > coding style point of view? > > > > if (info->bitmask & EBT_STP_ROOTADDR) { > > verdict = 0; > > for (i = 0; i < 6; i++) > > - verdict |= (stpc->root[2+i] ^ c->root_addr[i]) & > > - c->root_addrmsk[i]; > > + verdict |= (stpc->root[2 + i] ^ c->root_addr[i]) & > > + c->root_addrmsk[i]; > > > > I think the previous line is fine. > > "2+i" or "2 + i", either is OK. > Multiple line statement alignment doesn't > matter much. Sorry, I was actually refering to: > > + verdict |= (stpc->root[2 + i] ^ c->root_addr[i]) & > > + c->root_addrmsk[i]; ^^^ instead of: > > - verdict |= (stpc->root[2+i] ^ c->root_addr[i]) & > > - c->root_addrmsk[i]; ^ here. > I think either is fine and both are "don't care, don't need" > to change from one to another to satisfy some silly whitespace > overlord brainless script. > > Perhaps it's better to add a function for this though. I like this function idea :).
On Wed, 2016-06-08 at 13:52 +0200, Pablo Neira Ayuso wrote: > On Tue, Jun 07, 2016 at 11:02:30AM -0700, Joe Perches wrote: > > On Tue, 2016-06-07 at 19:34 +0200, Pablo Neira Ayuso wrote: > > > On Tue, Jun 07, 2016 at 10:04:40AM -0700, Joe Perches wrote: > > > > One more question, is this chunk below correct from > > > > coding style point of view? > > > if (info->bitmask & EBT_STP_ROOTADDR) { > > > verdict = 0; > > > for (i = 0; i < 6; i++) > > > - verdict |= (stpc->root[2+i] ^ c->root_addr[i]) & > > > - c->root_addrmsk[i]; > > > + verdict |= (stpc->root[2 + i] ^ c->root_addr[i]) & > > > + c->root_addrmsk[i]; > > > > > > I think the previous line is fine. > > "2+i" or "2 + i", either is OK. > > Multiple line statement alignment doesn't > > matter much. > Sorry, I was actually refering to: [] Hi again Pablo. No worries. I hoped the "doesn't matter much" was clear enough. There are many different multiple line statement alignment styles in the kernel. Alignment to open parenthesis is one of them, and I think it's reasonable to standardize on that. For multiple line statements without parentheses for alignment, I think there isn't one style that's much better than another. I slightly prefer the original alignment above myself. > > Perhaps it's better to add a function for this though. > I like this function idea :). Maybe something like this is clearer: static bool ebt_test_addr(const uint8_t *root, const char *addr, const char *mask) { int i; for (i = 0; i < ETH_ALEN; i++) { if ((root[2 + i] ^ addr[i]) & mask[i]) return true; } return false; } Maybe the call should add the + 2 to the first argument instead of using + 2 in the loop.
On Wed, Jun 08, 2016 at 09:52:30AM -0700, Joe Perches wrote: > On Wed, 2016-06-08 at 13:52 +0200, Pablo Neira Ayuso wrote: > > On Tue, Jun 07, 2016 at 11:02:30AM -0700, Joe Perches wrote: > > > On Tue, 2016-06-07 at 19:34 +0200, Pablo Neira Ayuso wrote: > > > > On Tue, Jun 07, 2016 at 10:04:40AM -0700, Joe Perches wrote: > > > > > One more question, is this chunk below correct from > > > > > coding style point of view? > > > > if (info->bitmask & EBT_STP_ROOTADDR) { > > > > verdict = 0; > > > > for (i = 0; i < 6; i++) > > > > - verdict |= (stpc->root[2+i] ^ c->root_addr[i]) & > > > > - c->root_addrmsk[i]; > > > > + verdict |= (stpc->root[2 + i] ^ c->root_addr[i]) & > > > > + c->root_addrmsk[i]; > > > > > > > > I think the previous line is fine. > > > "2+i" or "2 + i", either is OK. > > > Multiple line statement alignment doesn't > > > matter much. > > Sorry, I was actually refering to: > [] > > Hi again Pablo. > > No worries. I hoped the "doesn't matter much" was clear enough. > > There are many different multiple line statement alignment > styles in the kernel. > > Alignment to open parenthesis is one of them, and I think it's > reasonable to standardize on that. > > For multiple line statements without parentheses for alignment, > I think there isn't one style that's much better than another. > > I slightly prefer the original alignment above myself. I do too. I can take Tobin's original patch and manually revert this chunk then, ie. - verdict |= (stpc->root[2+i] ^ c->root_addr[i]) & - c->root_addrmsk[i]; + verdict |= (stpc->root[2 + i] ^ c->root_addr[i]) & + c->root_addrmsk[i]; > Maybe something like this is clearer: > > static bool ebt_test_addr(const uint8_t *root, const char *addr, > const char *mask) > { > int i; > > for (i = 0; i < ETH_ALEN; i++) { > if ((root[2 + i] ^ addr[i]) & mask[i]) > return true; > } > > return false; > } > > Maybe the call should add the + 2 to the first argument > instead of using + 2 in the loop. Then you can follow up with a patch to add this function. Just a suggestion, let me know if this is fine with you.
On Wed, Jun 08, 2016 at 07:31:21PM +0200, Pablo Neira Ayuso wrote: > Then you can follow up with a patch to add this function. > > Just a suggestion, let me know if this is fine with you. Forget this idea. Actually your patch from: Date: Tue, 07 Jun 2016 11:02:30 -0700 looks easier to readable than original Tobin's, so I'll wait for you to resubmit. Thanks.
On Wed, 2016-06-08 at 19:38 +0200, Pablo Neira Ayuso wrote: > On Wed, Jun 08, 2016 at 07:31:21PM +0200, Pablo Neira Ayuso wrote: > > Then you can follow up with a patch to add this function. > > > > Just a suggestion, let me know if this is fine with you. > Forget this idea. > > Actually your patch from: Date: Tue, 07 Jun 2016 11:02:30 -0700 > > looks easier to readable than original Tobin's, so I'll wait for you > to resubmit. Well, maybe next week after the other couple patches are in -next.
On Thu, Jun 09, 2016 at 11:00:18AM -0700, Joe Perches wrote: > On Wed, 2016-06-08 at 19:38 +0200, Pablo Neira Ayuso wrote: > > On Wed, Jun 08, 2016 at 07:31:21PM +0200, Pablo Neira Ayuso wrote: > > looks easier to readable than original Tobin's, so I'll wait for you > > to resubmit. > > Well, maybe next week after the other couple patches are in -next. > Not sure if I should have been adding anything to this thread since I submitted the original patch. I'm new to the LKML and have not been able to follow the subtleties of conversation in this thread about who wants who to do what or if anyone needs to do anything. If there is any follow up that is required (or requested) of me I am more than happy to do so. I hope I have not created unnecessary work for you fellas by adding this whitespace patch to the set. I will be more cautious about making changes to satisfy checkpatch.pl in future. thanks, Tobin.
diff --git a/net/bridge/netfilter/ebt_stp.c b/net/bridge/netfilter/ebt_stp.c index 6b731e1..4096fac 100644 --- a/net/bridge/netfilter/ebt_stp.c +++ b/net/bridge/netfilter/ebt_stp.c @@ -40,13 +40,24 @@ struct stp_config_pdu { #define NR16(p) (p[0] << 8 | p[1]) #define NR32(p) ((p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3]) +static bool ebt_test_addr(const uint8_t *root, const char *addr, + const char *mask) +{ + bool rtn = false; + int i; + + for (i = 0; i < ETH_ALEN; i++) + rtn |= (root[2 + i] ^ addr[i]) & mask[i]; + + return rtn; +} + static bool ebt_filter_config(const struct ebt_stp_info *info, const struct stp_config_pdu *stpc) { const struct ebt_stp_config_info *c; uint16_t v16; uint32_t v32; - int verdict, i; c = &info->config; if ((info->bitmask & EBT_STP_FLAGS) && @@ -54,66 +65,61 @@ static bool ebt_filter_config(const struct ebt_stp_info *info, return false; if (info->bitmask & EBT_STP_ROOTPRIO) { v16 = NR16(stpc->root); - if (FWINV(v16 < c->root_priol || - v16 > c->root_priou, EBT_STP_ROOTPRIO)) + if (FWINV(v16 < c->root_priol || v16 > c->root_priou, + EBT_STP_ROOTPRIO)) return false; } if (info->bitmask & EBT_STP_ROOTADDR) { - verdict = 0; - for (i = 0; i < 6; i++) - verdict |= (stpc->root[2+i] ^ c->root_addr[i]) & - c->root_addrmsk[i]; - if (FWINV(verdict != 0, EBT_STP_ROOTADDR)) + if (FWINV(ebt_test_addr(stpc->root, c->root_addr, + c->root_addrmsk), + EBT_STP_ROOTADDR)) return false; } if (info->bitmask & EBT_STP_ROOTCOST) { v32 = NR32(stpc->root_cost); - if (FWINV(v32 < c->root_costl || - v32 > c->root_costu, EBT_STP_ROOTCOST)) + if (FWINV(v32 < c->root_costl || v32 > c->root_costu, + EBT_STP_ROOTCOST)) return false; } if (info->bitmask & EBT_STP_SENDERPRIO) { v16 = NR16(stpc->sender); - if (FWINV(v16 < c->sender_priol || - v16 > c->sender_priou, EBT_STP_SENDERPRIO)) + if (FWINV(v16 < c->sender_priol || v16 > c->sender_priou, + EBT_STP_SENDERPRIO)) return false; } if (info->bitmask & EBT_STP_SENDERADDR) { - verdict = 0; - for (i = 0; i < 6; i++) - verdict |= (stpc->sender[2+i] ^ c->sender_addr[i]) & - c->sender_addrmsk[i]; - if (FWINV(verdict != 0, EBT_STP_SENDERADDR)) + if (FWINV(ebt_test_addr(stpc->sender, c->sender_addr, + c->sender_addrmsk), + EBT_STP_SENDERADDR)) return false; } if (info->bitmask & EBT_STP_PORT) { v16 = NR16(stpc->port); - if (FWINV(v16 < c->portl || - v16 > c->portu, EBT_STP_PORT)) + if (FWINV(v16 < c->portl || v16 > c->portu, EBT_STP_PORT)) return false; } if (info->bitmask & EBT_STP_MSGAGE) { v16 = NR16(stpc->msg_age); - if (FWINV(v16 < c->msg_agel || - v16 > c->msg_ageu, EBT_STP_MSGAGE)) + if (FWINV(v16 < c->msg_agel || v16 > c->msg_ageu, + EBT_STP_MSGAGE)) return false; } if (info->bitmask & EBT_STP_MAXAGE) { v16 = NR16(stpc->max_age); - if (FWINV(v16 < c->max_agel || - v16 > c->max_ageu, EBT_STP_MAXAGE)) + if (FWINV(v16 < c->max_agel || v16 > c->max_ageu, + EBT_STP_MAXAGE)) return false; } if (info->bitmask & EBT_STP_HELLOTIME) { v16 = NR16(stpc->hello_time); - if (FWINV(v16 < c->hello_timel || - v16 > c->hello_timeu, EBT_STP_HELLOTIME)) + if (FWINV(v16 < c->hello_timel || v16 > c->hello_timeu, + EBT_STP_HELLOTIME)) return false; } if (info->bitmask & EBT_STP_FWDD) { v16 = NR16(stpc->forward_delay); - if (FWINV(v16 < c->forward_delayl || - v16 > c->forward_delayu, EBT_STP_FWDD)) + if (FWINV(v16 < c->forward_delayl || v16 > c->forward_delayu, + EBT_STP_FWDD)) return false; } return true;