* [PATCH] bonding: use pre-defined macro in bond_mode_name instead of magic number 0 @ 2013-07-24 6:53 Wang Sheng-Hui 2013-07-24 7:10 ` Joe Perches 2013-07-26 20:54 ` David Miller 0 siblings, 2 replies; 7+ messages in thread From: Wang Sheng-Hui @ 2013-07-24 6:53 UTC (permalink / raw) To: Jay Vosburgh, Andy Gospodarek, netdev We have BOND_MODE_ROUNDROBIN pre-defined as 0, and it's the lowest mode number. Use it to check the arg lower bound instead of magic number 0 in bond_mode_name. Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com> --- drivers/net/bonding/bond_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 07f257d4..5890def 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -273,7 +273,7 @@ const char *bond_mode_name(int mode) [BOND_MODE_ALB] = "adaptive load balancing", }; - if (mode < 0 || mode > BOND_MODE_ALB) + if (mode < BOND_MODE_ROUNDROBIN || mode > BOND_MODE_ALB) return "unknown"; return names[mode]; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] bonding: use pre-defined macro in bond_mode_name instead of magic number 0 2013-07-24 6:53 [PATCH] bonding: use pre-defined macro in bond_mode_name instead of magic number 0 Wang Sheng-Hui @ 2013-07-24 7:10 ` Joe Perches 2013-07-24 12:38 ` Wang Sheng-Hui 2013-07-26 20:53 ` David Miller 2013-07-26 20:54 ` David Miller 1 sibling, 2 replies; 7+ messages in thread From: Joe Perches @ 2013-07-24 7:10 UTC (permalink / raw) To: Wang Sheng-Hui; +Cc: Jay Vosburgh, Andy Gospodarek, netdev On Wed, 2013-07-24 at 14:53 +0800, Wang Sheng-Hui wrote: > We have BOND_MODE_ROUNDROBIN pre-defined as 0, and it's the lowest mode number. > Use it to check the arg lower bound instead of magic number 0 in bond_mode_name. [] > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c [] > @@ -273,7 +273,7 @@ const char *bond_mode_name(int mode) > [BOND_MODE_ALB] = "adaptive load balancing", > }; > > - if (mode < 0 || mode > BOND_MODE_ALB) > + if (mode < BOND_MODE_ROUNDROBIN || mode > BOND_MODE_ALB) > return "unknown"; > > return names[mode]; Probably be simpler, less confusing, and more normal style to use a switch case. switch (mode) { case BOND_MODE_ROUNDROBIN: return "load balancing (round-robin)"; case BOND_MODE_ACTIVEBACKUP: return "fault-tolerance (active-backup)"; case BOND_MODE_XOR: return "load balancing (xor)"; case BOND_MODE_BROADCAST; return "fault-tolerance (broadcast)"; case BOND_MODE_8023AD: return "IEEE 802.3ad Dynamic link aggregation"; case BOND_MODE_TLB: return "transmit load balancing"; case BOND_MODE_ALB: return "adaptive load balancing"; default: return "unknown"; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bonding: use pre-defined macro in bond_mode_name instead of magic number 0 2013-07-24 7:10 ` Joe Perches @ 2013-07-24 12:38 ` Wang Sheng-Hui 2013-07-26 20:53 ` David Miller 1 sibling, 0 replies; 7+ messages in thread From: Wang Sheng-Hui @ 2013-07-24 12:38 UTC (permalink / raw) To: Joe Perches; +Cc: Jay Vosburgh, Andy Gospodarek, netdev On 2013年07月24日 15:10, Joe Perches wrote: > On Wed, 2013-07-24 at 14:53 +0800, Wang Sheng-Hui wrote: >> We have BOND_MODE_ROUNDROBIN pre-defined as 0, and it's the lowest mode number. >> Use it to check the arg lower bound instead of magic number 0 in bond_mode_name. > [] >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > [] >> @@ -273,7 +273,7 @@ const char *bond_mode_name(int mode) >> [BOND_MODE_ALB] = "adaptive load balancing", >> }; >> >> - if (mode < 0 || mode > BOND_MODE_ALB) >> + if (mode < BOND_MODE_ROUNDROBIN || mode > BOND_MODE_ALB) >> return "unknown"; >> >> return names[mode]; > > Probably be simpler, less confusing, and more normal style > to use a switch case. > > switch (mode) { > case BOND_MODE_ROUNDROBIN: > return "load balancing (round-robin)"; > case BOND_MODE_ACTIVEBACKUP: > return "fault-tolerance (active-backup)"; > case BOND_MODE_XOR: > return "load balancing (xor)"; > case BOND_MODE_BROADCAST; > return "fault-tolerance (broadcast)"; > case BOND_MODE_8023AD: > return "IEEE 802.3ad Dynamic link aggregation"; > case BOND_MODE_TLB: > return "transmit load balancing"; > case BOND_MODE_ALB: > return "adaptive load balancing"; > default: > return "unknown"; > } > > Regenerated the patch on Joe's advice. Please check it. Thanks, [PATCH] bonding: use switch case to make bond_mode_name more clear We have predefined the mode macros, and should avoid the magic number 0 in the old version. Also, use a switch case would be simpler, less confusing, and more normal style. Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com> --- drivers/net/bonding/bond_main.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 07f257d4..69bfb4c 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -263,20 +263,24 @@ static void bond_uninit(struct net_device *bond_dev); const char *bond_mode_name(int mode) { - static const char *names[] = { - [BOND_MODE_ROUNDROBIN] = "load balancing (round-robin)", - [BOND_MODE_ACTIVEBACKUP] = "fault-tolerance (active-backup)", - [BOND_MODE_XOR] = "load balancing (xor)", - [BOND_MODE_BROADCAST] = "fault-tolerance (broadcast)", - [BOND_MODE_8023AD] = "IEEE 802.3ad Dynamic link aggregation", - [BOND_MODE_TLB] = "transmit load balancing", - [BOND_MODE_ALB] = "adaptive load balancing", - }; - - if (mode < 0 || mode > BOND_MODE_ALB) + switch (mode) { + case BOND_MODE_ROUNDROBIN: + return "load balancing (round-robin)"; + case BOND_MODE_ACTIVEBACKUP: + return "fault-tolerance (active-backup)"; + case BOND_MODE_XOR: + return "load balancing (xor)"; + case BOND_MODE_BROADCAST: + return "fault-tolerance (broadcast)"; + case BOND_MODE_8023AD: + return "IEEE 802.3ad Dynamic link aggregation"; + case BOND_MODE_TLB: + return "transmit load balancing"; + case BOND_MODE_ALB: + return "adaptive load balancing"; + default: return "unknown"; - - return names[mode]; + } } /*---------------------------------- VLAN -----------------------------------*/ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] bonding: use pre-defined macro in bond_mode_name instead of magic number 0 2013-07-24 7:10 ` Joe Perches 2013-07-24 12:38 ` Wang Sheng-Hui @ 2013-07-26 20:53 ` David Miller 2013-07-26 20:56 ` Joe Perches 2013-07-26 21:25 ` Joe Perches 1 sibling, 2 replies; 7+ messages in thread From: David Miller @ 2013-07-26 20:53 UTC (permalink / raw) To: joe; +Cc: shhuiw, fubar, andy, netdev From: Joe Perches <joe@perches.com> Date: Wed, 24 Jul 2013 00:10:19 -0700 > Probably be simpler, less confusing, and more normal style > to use a switch case. > > switch (mode) { > case BOND_MODE_ROUNDROBIN: > return "load balancing (round-robin)"; > case BOND_MODE_ACTIVEBACKUP: > return "fault-tolerance (active-backup)"; > case BOND_MODE_XOR: > return "load balancing (xor)"; > case BOND_MODE_BROADCAST; > return "fault-tolerance (broadcast)"; > case BOND_MODE_8023AD: > return "IEEE 802.3ad Dynamic link aggregation"; > case BOND_MODE_TLB: > return "transmit load balancing"; > case BOND_MODE_ALB: > return "adaptive load balancing"; > default: > return "unknown"; > } I have to say that I do not prefer this approach. I have a good idea what the compiler ends up outputting for the above, and the "indexed table of strings" approach is much less code. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bonding: use pre-defined macro in bond_mode_name instead of magic number 0 2013-07-26 20:53 ` David Miller @ 2013-07-26 20:56 ` Joe Perches 2013-07-26 21:25 ` Joe Perches 1 sibling, 0 replies; 7+ messages in thread From: Joe Perches @ 2013-07-26 20:56 UTC (permalink / raw) To: David Miller; +Cc: shhuiw, fubar, andy, netdev On Fri, 2013-07-26 at 13:53 -0700, David Miller wrote: > From: Joe Perches <joe@perches.com> > Date: Wed, 24 Jul 2013 00:10:19 -0700 > > > Probably be simpler, less confusing, and more normal style > > to use a switch case. > > > > switch (mode) { > > case BOND_MODE_ROUNDROBIN: > > return "load balancing (round-robin)"; > > case BOND_MODE_ACTIVEBACKUP: > > return "fault-tolerance (active-backup)"; > > case BOND_MODE_XOR: > > return "load balancing (xor)"; > > case BOND_MODE_BROADCAST; > > return "fault-tolerance (broadcast)"; > > case BOND_MODE_8023AD: > > return "IEEE 802.3ad Dynamic link aggregation"; > > case BOND_MODE_TLB: > > return "transmit load balancing"; > > case BOND_MODE_ALB: > > return "adaptive load balancing"; > > default: > > return "unknown"; > > } > > I have to say that I do not prefer this approach. > > I have a good idea what the compiler ends up outputting for the above, > and the "indexed table of strings" approach is much less code. It has the benefit of not breaking if ever the BOND_MODE_<foo> #defines are ever non-consecutive. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bonding: use pre-defined macro in bond_mode_name instead of magic number 0 2013-07-26 20:53 ` David Miller 2013-07-26 20:56 ` Joe Perches @ 2013-07-26 21:25 ` Joe Perches 1 sibling, 0 replies; 7+ messages in thread From: Joe Perches @ 2013-07-26 21:25 UTC (permalink / raw) To: David Miller; +Cc: shhuiw, fubar, andy, netdev On Fri, 2013-07-26 at 13:53 -0700, David Miller wrote: > From: Joe Perches <joe@perches.com> > Date: Wed, 24 Jul 2013 00:10:19 -0700 > > > Probably be simpler, less confusing, and more normal style > > to use a switch case. > > > > switch (mode) { > > case BOND_MODE_ROUNDROBIN: > > return "load balancing (round-robin)"; > > case BOND_MODE_ACTIVEBACKUP: > > return "fault-tolerance (active-backup)"; > > case BOND_MODE_XOR: > > return "load balancing (xor)"; > > case BOND_MODE_BROADCAST; > > return "fault-tolerance (broadcast)"; > > case BOND_MODE_8023AD: > > return "IEEE 802.3ad Dynamic link aggregation"; > > case BOND_MODE_TLB: > > return "transmit load balancing"; > > case BOND_MODE_ALB: > > return "adaptive load balancing"; > > default: > > return "unknown"; > > } > > I have to say that I do not prefer this approach. > > I have a good idea what the compiler ends up outputting for the above, > and the "indexed table of strings" approach is much less code. > just fyi: it doesn't seem like "much less" to at least gcc 4.7 $ cat t1.c #include <stdio.h> #include <stdlib.h> const char * __attribute__((noinline)) string1(int index) { static const char *strings[] = { [0] = "String10", [1] = "String11", [2] = "String12", [3] = "String13", [4] = "String14", [5] = "String15", }; if (index < 0 || index > 5) return NULL; return strings[index]; } const char * __attribute__((noinline)) string2(int index) { switch (index) { case 0: return "String20"; case 1: return "String21"; case 2: return "String22"; case 3: return "String23"; case 4: return "String24"; case 5: return "String25"; } return NULL; } int main(int argc, char** argv) { int val; srand(time(NULL)); val = rand()%6; printf("1: %s, 2: %s\n", string1(val), string2(val)); return 0; } $ gcc -S -O2 t1.c $ cat t1.s .file "t1.c" .text .p2align 4,,15 .globl string1 .type string1, @function string1: .LFB38: .cfi_startproc movl 4(%esp), %eax cmpl $5, %eax ja .L3 movl strings.2633(,%eax,4), %eax ret .p2align 4,,7 .p2align 3 .L3: xorl %eax, %eax ret .cfi_endproc .LFE38: .size string1, .-string1 .p2align 4,,15 .globl string2 .type string2, @function string2: .LFB39: .cfi_startproc movl 4(%esp), %edx xorl %eax, %eax cmpl $5, %edx ja .L6 movl CSWTCH.5(,%edx,4), %eax .L6: rep ret .cfi_endproc .LFE39: .size string2, .-string2 .section .rodata.str1.1,"aMS",@progbits,1 .LC0: .string "1: %s, 2: %s\n" .section .text.startup,"ax",@progbits .p2align 4,,15 .globl main .type main, @function main: .LFB40: .cfi_startproc pushl %ebp .cfi_def_cfa_offset 8 .cfi_offset 5, -8 movl %esp, %ebp .cfi_def_cfa_register 5 pushl %esi pushl %ebx .cfi_offset 6, -12 .cfi_offset 3, -16 movl $715827883, %ebx andl $-16, %esp subl $16, %esp movl $0, (%esp) call time movl %eax, (%esp) call srand call rand movl %eax, %ecx imull %ebx movl %ecx, %eax sarl $31, %eax movl %edx, %ebx subl %eax, %ebx leal (%ebx,%ebx,2), %eax movl %ecx, %ebx addl %eax, %eax subl %eax, %ebx movl %ebx, (%esp) call string2 movl %ebx, (%esp) movl %eax, %esi call string1 movl %esi, 12(%esp) movl $.LC0, 4(%esp) movl $1, (%esp) movl %eax, 8(%esp) call __printf_chk leal -8(%ebp), %esp xorl %eax, %eax popl %ebx .cfi_restore 3 popl %esi .cfi_restore 6 popl %ebp .cfi_restore 5 .cfi_def_cfa 4, 4 ret .cfi_endproc .LFE40: .size main, .-main .section .rodata.str1.1 .LC1: .string "String10" .LC2: .string "String11" .LC3: .string "String12" .LC4: .string "String13" .LC5: .string "String14" .LC6: .string "String15" .section .rodata .align 4 .type strings.2633, @object .size strings.2633, 24 strings.2633: .long .LC1 .long .LC2 .long .LC3 .long .LC4 .long .LC5 .long .LC6 .section .rodata.str1.1 .LC7: .string "String20" .LC8: .string "String21" .LC9: .string "String22" .LC10: .string "String23" .LC11: .string "String24" .LC12: .string "String25" .section .rodata .align 4 .type CSWTCH.5, @object .size CSWTCH.5, 24 CSWTCH.5: .long .LC7 .long .LC8 .long .LC9 .long .LC10 .long .LC11 .long .LC12 .ident "GCC: (Ubuntu/Linaro 4.7.3-1ubuntu1) 4.7.3" .section .note.GNU-stack,"",@progbits ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bonding: use pre-defined macro in bond_mode_name instead of magic number 0 2013-07-24 6:53 [PATCH] bonding: use pre-defined macro in bond_mode_name instead of magic number 0 Wang Sheng-Hui 2013-07-24 7:10 ` Joe Perches @ 2013-07-26 20:54 ` David Miller 1 sibling, 0 replies; 7+ messages in thread From: David Miller @ 2013-07-26 20:54 UTC (permalink / raw) To: shhuiw; +Cc: fubar, andy, netdev From: Wang Sheng-Hui <shhuiw@gmail.com> Date: Wed, 24 Jul 2013 14:53:26 +0800 > We have BOND_MODE_ROUNDROBIN pre-defined as 0, and it's the lowest > mode number. > Use it to check the arg lower bound instead of magic number 0 in > bond_mode_name. > > Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com> > --- > drivers/net/bonding/bond_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/bonding/bond_main.c > b/drivers/net/bonding/bond_main.c > index 07f257d4..5890def 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -273,7 +273,7 @@ const char *bond_mode_name(int mode) > [BOND_MODE_ALB] = "adaptive load balancing", > }; > > - if (mode < 0 || mode > BOND_MODE_ALB) > + if (mode < BOND_MODE_ROUNDROBIN || mode > BOND_MODE_ALB) > return "unknown"; > > return names[mode]; I applied this version of your patch, thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-07-26 21:26 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-24 6:53 [PATCH] bonding: use pre-defined macro in bond_mode_name instead of magic number 0 Wang Sheng-Hui 2013-07-24 7:10 ` Joe Perches 2013-07-24 12:38 ` Wang Sheng-Hui 2013-07-26 20:53 ` David Miller 2013-07-26 20:56 ` Joe Perches 2013-07-26 21:25 ` Joe Perches 2013-07-26 20:54 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).