| Message ID | 1353759675-30511-1-git-send-email-nikolay@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Nikolay Aleksandrov <nikolay@redhat.com> Date: Sat, 24 Nov 2012 13:21:15 +0100 > @@ -4706,12 +4706,14 @@ static int bond_check_params(struct bond_params *params) > arp_ip_count++) { > /* not complete check, but should be good enough to > catch mistakes */ > - if (!isdigit(arp_ip_target[arp_ip_count][0])) { > + __be32 ip = in_aton(arp_ip_target[arp_ip_count]); > + if (!isdigit(arp_ip_target[arp_ip_count][0]) > + || ip == 0 > + || ip == htonl(INADDR_BROADCAST)) { Please format this properly, put the connecting operators at the end, not the beginning, of the lines of the if statement, like so: if (!isdigit(arp_ip_target[arp_ip_count][0]) || ip == 0 || ip == htonl(INADDR_BROADCAST)) { Where else did you see the layout you used? It's not a prevalent construct, so as far as I can tell you came up with it on your own. Please don't do this, and instead use existing practice as your guide. 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
Actually I did see it from kernel/module.c and didn't come up with it myself. Anyway I will re-post with the change and will take a note for the future. Thanks for the review On 28/11/12 17:23, David Miller wrote: > From: Nikolay Aleksandrov<nikolay@redhat.com> > Date: Sat, 24 Nov 2012 13:21:15 +0100 > >> @@ -4706,12 +4706,14 @@ static int bond_check_params(struct bond_params *params) >> arp_ip_count++) { >> /* not complete check, but should be good enough to >> catch mistakes */ >> - if (!isdigit(arp_ip_target[arp_ip_count][0])) { >> + __be32 ip = in_aton(arp_ip_target[arp_ip_count]); >> + if (!isdigit(arp_ip_target[arp_ip_count][0]) >> + || ip == 0 >> + || ip == htonl(INADDR_BROADCAST)) { > Please format this properly, put the connecting operators at the end, > not the beginning, of the lines of the if statement, like so: > > if (!isdigit(arp_ip_target[arp_ip_count][0]) || > ip == 0 || > ip == htonl(INADDR_BROADCAST)) { > > Where else did you see the layout you used? It's not a prevalent > construct, so as far as I can tell you came up with it on your own. > > Please don't do this, and instead use existing practice as your guide. > > 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
Nikolay Aleksandrov <nikolay@redhat.com> writes: > Actually I did see it from kernel/module.c and didn't come up with it > myself. Just FYI, running scripts/checkpatch.pl -f kernel/module.c results in: total: 27 errors, 65 warnings, 3639 lines checked NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or scripts/cleanfile so I'd not use that as a style guide if I were you.... Bjørn -- 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/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 5f5b69f..940eac8 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -4706,12 +4706,14 @@ static int bond_check_params(struct bond_params *params) arp_ip_count++) { /* not complete check, but should be good enough to catch mistakes */ - if (!isdigit(arp_ip_target[arp_ip_count][0])) { + __be32 ip = in_aton(arp_ip_target[arp_ip_count]); + if (!isdigit(arp_ip_target[arp_ip_count][0]) + || ip == 0 + || ip == htonl(INADDR_BROADCAST)) { pr_warning("Warning: bad arp_ip_target module parameter (%s), ARP monitoring will not be performed\n", arp_ip_target[arp_ip_count]); arp_interval = 0; } else { - __be32 ip = in_aton(arp_ip_target[arp_ip_count]); arp_target[arp_ip_count] = ip; } }
The module can be loaded with arp_ip_target="255.255.255.255" which makes it impossible to remove as the function in sysfs checks for that value, so we make the parameter checks consistent with sysfs. Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com> --- drivers/net/bonding/bond_main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)