bonding: use pre-defined macro in bond_mode_name instead of magic number 0

PATCH Applied Wang Sheng-Hui 3 participants 7 messages Jul 24, 2013 – Jul 26, 2013

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Wang Sheng-Hui

author

2013-07-24 06:53 UTC #0 lore raw

PATCH 1/1 bonding: use pre-defined macro in bond_mode_name instead of magic number 0

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 <[email protected]>

---
 drivers/net/bonding/bond_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/bonding/bond_main.c +1 -1

index 07f257d4..5890def 100644
@@ -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];

Signature · 1 line

1.7.10.4

Joe Perches

2013-07-24 07:10 UTC #1 lore raw

Quoted 2 lines · Wang Sheng-Hui

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.

[]

drivers/net/bonding/bond_main.c

Quoted 1 line

drivers/net/bonding/bond_main.c +0 -0

[]

drivers/net/bonding/bond_main.c const char *bond_mode_name(int mode) · line 279

Quoted 9 lines

drivers/net/bonding/bond_main.c +1 -1

@@ -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";
	}

Wang Sheng-Hui

author

2013-07-24 12:38 UTC #2 lore raw

Quoted 29 lines · Joe Perches

Quoted 2 lines · Wang Sheng-Hui
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.
[]
drivers/net/bonding/bond_main.c
Quoted 1 line
drivers/net/bonding/bond_main.c +0 -0
[]
drivers/net/bonding/bond_main.c const char *bond_mode_name(int mode) · line 279
Quoted 9 lines
drivers/net/bonding/bond_main.c +1 -1
@@ -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:

Quoted 10 lines

		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 <[email protected]>

---
 drivers/net/bonding/bond_main.c |   30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

drivers/net/bonding/bond_main.c +17 ~~-13~~

index 07f257d4..69bfb4c 100644
@@ -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 -----------------------------------*/

Signature · 1 line

1.7.10.4

David Miller

maintainer

2013-07-26 20:53 UTC #3 lore raw

In reply to Joe Perches

From: Joe Perches <[email protected]>
Date: Wed, 24 Jul 2013 00:10:19 -0700

Quoted 13 lines

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:

Quoted 8 lines

		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.

Joe Perches

2013-07-26 20:56 UTC #4 lore raw

Quoted 25 lines · David Miller

From: Joe Perches <[email protected]>
Date: Wed, 24 Jul 2013 00:10:19 -0700

Quoted 13 lines

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:

Quoted 8 lines

              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";
      }

Quoted 4 lines

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.

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

David Miller

maintainer Applied

2013-07-26 20:54 UTC #6 lore raw

In reply to Wang Sheng-Hui

From: Wang Sheng-Hui <[email protected]>
Date: Wed, 24 Jul 2013 14:53:26 +0800

Quoted 16 lines

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 <[email protected]>
---
 drivers/net/bonding/bond_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/bonding/bond_main.c +0 -0
index 07f257d4..5890def 100644

drivers/net/bonding/bond_main.c const char *bond_mode_name(int mode) · line 279

Quoted 8 lines

drivers/net/bonding/bond_main.c +1 -1

@@ -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.