Skip to contentSkip to conversation

kvm tools: Use assert() helper to check a variable value

PATCH In review Cyrill Gorcunov 3 participants 12 messages Dec 18, 2011 – Dec 19, 2011 KVM

lore
Share & tools
lore

Cyrill Gorcunov

author
#0 lore raw

PATCH 1/1 kvm tools: Use assert() helper to check a variable value

BUILD_BUG_ON is unable to catch errors on expression which can't be evaluated at compile time.

Signed-off-by Cyrill Gorcunov <[email protected]>
---
 tools/kvm/x86/bios.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
tools/kvm/x86/bios.c +2 -1
Index: linux-2.6.git/tools/kvm/x86/bios.c
===================================================================
@@ -5,6 +5,7 @@
 #include "kvm/util.h"
 
 #include <string.h>
+#include <assert.h>
 #include <asm/e820.h>
 
 #include "bios/bios-rom.h"

@@ -98,7 +99,7 @@ static void e820_setup(struct kvm *kvm)
 		};
 	}
 
-	BUILD_BUG_ON(i > E820_X_MAX);
+	assert(i <= E820_X_MAX);
 
 	e820->nr_map = i;
 }
#1 lore raw
Quoted 21 lines · Cyrill Gorcunov

BUILD_BUG_ON is unable to catch errors on expression which can't be evaluated at compile time.

Signed-off-by Cyrill Gorcunov <[email protected]>

---
tools/kvm/x86/bios.c | 3 ++-

1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.git/tools/kvm/x86/bios.c
===================================================================

tools/kvm/x86/bios.c +1 -0
@@ -5,6 +5,7 @@
 #include "kvm/util.h"
 
 #include <string.h>
+#include <assert.h>
 #include <asm/e820.h>
 
 #include "bios/bios-rom.h"

tools/kvm/x86/bios.c static void e820_setup(struct kvm *kvm) · line 102

Quoted 5 lines
tools/kvm/x86/bios.c +1 -1
@@ -98,7 +99,7 @@ static void e820_setup(struct kvm *kvm)
 		};
 	}
 
-	BUILD_BUG_ON(i > E820_X_MAX);
+	assert(i <= E820_X_MAX);

We should use BUG_ON() like tools/perf does.

author
#2 lore raw
Quoted 6 lines · Pekka Enberg

tools/kvm/x86/bios.c static void e820_setup(struct kvm *kvm) · line 102

Quoted 3 lines
tools/kvm/x86/bios.c +1 -1
-	BUILD_BUG_ON(i > E820_X_MAX);
+	assert(i <= E820_X_MAX);

We should use BUG_ON() like tools/perf does.

We dont have it yet. So I'll introduce this helper later, but note that we will have to cover _all_ assert() calls then, so it's better to make in a separate patch. Meanwhile such fix it better than bug ;)

Cyrill

#3 lore raw
Quoted 11 lines · Cyrill Gorcunov
Quoted 6 lines · Pekka Enberg

tools/kvm/x86/bios.c static void e820_setup(struct kvm *kvm) · line 102

Quoted 3 lines
tools/kvm/x86/bios.c +1 -1
-	BUILD_BUG_ON(i > E820_X_MAX);
+	assert(i <= E820_X_MAX);

We should use BUG_ON() like tools/perf does.

We dont have it yet. So I'll introduce this helper later, but note that we will have to cover _all_ assert() calls then, so it's better to make in a separate patch. [...]

There's only about a dozen of them, should be easy.

Thanks,

Ingo

#4 lore raw
In reply to Cyrill Gorcunov
Quoted 5 lines · Pekka Enberg
Quoted 5 lines
Quoted 3 lines

-    BUILD_BUG_ON(i > E820_X_MAX);
+    assert(i <= E820_X_MAX);

We should use BUG_ON() like tools/perf does.

Quoted 4 lines · Cyrill Gorcunov

We dont have it yet. So I'll introduce this helper later, but note that we will have to cover _all_ assert() calls then, so it's better to make in a separate patch. Meanwhile such fix it better than bug ;)

You don't need to convert all of them at the same time but we're not adding new assert() calls.

author
#5 lore raw
Quoted 7 lines · Pekka Enberg
Quoted 5 lines · Pekka Enberg
Quoted 5 lines
Quoted 3 lines

-    BUILD_BUG_ON(i > E820_X_MAX);
+    assert(i <= E820_X_MAX);

We should use BUG_ON() like tools/perf does.

Quoted 9 lines
Quoted 4 lines · Cyrill Gorcunov

We dont have it yet. So I'll introduce this helper later, but note that we will have to cover _all_ assert() calls then, so it's better to make in a separate patch. Meanwhile such fix it better than bug ;)

You don't need to convert all of them at the same time but we're not adding new assert() calls.

You both (Pekka and Ingo) know the way how to convince people ;) Something like below?

Cyrill
---
kvm tools: Add BUG_ON() helper to make a run-time critical tests

Also drop useless assert.h inclusions.

Signed-off-by Cyrill Gorcunov <[email protected]>
---
 tools/kvm/include/kvm/util.h |    5 ++++-
 tools/kvm/ioport.c           |    1 -
 tools/kvm/kvm-cmd.c          |    6 ++----
 tools/kvm/kvm.c              |    1 -
 tools/kvm/pci.c              |    6 ++----
 tools/kvm/powerpc/kvm.c      |    1 -
 tools/kvm/virtio/console.c   |    3 +--
 tools/kvm/virtio/net.c       |    1 -
 tools/kvm/x86/bios.c         |    2 +-
 tools/kvm/x86/cpuid.c        |    1 -
 tools/kvm/x86/kvm.c          |    1 -
 11 files changed, 10 insertions(+), 18 deletions(-)
tools/kvm/include/kvm/util.h +4 -1
Index: linux-2.6.git/tools/kvm/include/kvm/util.h
===================================================================
@@ -9,6 +9,7 @@
  * Some bits are stolen from perf tool :)
  */
 
+#include <assert.h>
 #include <unistd.h>
 #include <stdio.h>
 #include <stddef.h>

@@ -50,7 +51,9 @@ extern void set_die_routine(void (*routi
 				__func__, __LINE__, ##__VA_ARGS__);	\
 	} while (0)
 
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#
+#define BUILD_BUG_ON(condition)	((void)sizeof(char[1 - 2*!!(condition)]))
+#define BUG_ON(condition)	assert(!(condition))
 
 #define DIE_IF(cnd)						\
 do {								\
tools/kvm/ioport.c +0 -1
Index: linux-2.6.git/tools/kvm/ioport.c
===================================================================
@@ -10,7 +10,6 @@
 #include <linux/types.h>
 
 #include <stdbool.h>
-#include <assert.h>
 #include <limits.h>
 #include <stdlib.h>
 #include <stdio.h>
tools/kvm/kvm-cmd.c +2 -4
Index: linux-2.6.git/tools/kvm/kvm-cmd.c
===================================================================
@@ -2,8 +2,6 @@
 #include <string.h>
 #include <errno.h>
 
-#include <assert.h>
-
 /* user defined header files */
 #include "kvm/builtin-debug.h"
 #include "kvm/builtin-pause.h"

@@ -71,14 +69,14 @@ int handle_command(struct cmd_struct *co
 
 	if (!argv || !*argv) {
 		p = kvm_get_command(command, "help");
-		assert(p);
+		BUG_ON(!p);
 		return p->fn(argc, argv, prefix);
 	}
 
 	p = kvm_get_command(command, argv[0]);
 	if (!p) {
 		p = kvm_get_command(command, "help");
-		assert(p);
+		BUG_ON(!p);
 		p->fn(0, NULL, prefix);
 		return EINVAL;
 	}
tools/kvm/kvm.c +0 -1
Index: linux-2.6.git/tools/kvm/kvm.c
===================================================================
@@ -14,7 +14,6 @@
 #include <sys/ioctl.h>
 #include <sys/mman.h>
 #include <stdbool.h>
-#include <assert.h>
 #include <limits.h>
 #include <signal.h>
 #include <stdarg.h>
tools/kvm/pci.c +2 -4
Index: linux-2.6.git/tools/kvm/pci.c
===================================================================
@@ -3,8 +3,6 @@
 #include "kvm/util.h"
 #include "kvm/kvm.h"
 
-#include <assert.h>
-
 #define PCI_BAR_OFFSET(b)		(offsetof(struct pci_device_header, bar[b]))
 
 static struct pci_device_header		*pci_devices[PCI_MAX_DEVICES];

@@ -170,13 +168,13 @@ void pci__config_rd(struct kvm *kvm, uni
 
 void pci__register(struct pci_device_header *dev, u8 dev_num)
 {
-	assert(dev_num < PCI_MAX_DEVICES);
+	BUG_ON(dev_num >= PCI_MAX_DEVICES);
 	pci_devices[dev_num]	= dev;
 }
 
 struct pci_device_header *pci__find_dev(u8 dev_num)
 {
-	assert(dev_num < PCI_MAX_DEVICES);
+	BUG_ON(dev_num >= PCI_MAX_DEVICES);
 	return pci_devices[dev_num];
 }
tools/kvm/powerpc/kvm.c +0 -1
Index: linux-2.6.git/tools/kvm/powerpc/kvm.c
===================================================================
@@ -17,7 +17,6 @@
 #include <sys/ioctl.h>
 #include <sys/mman.h>
 #include <stdbool.h>
-#include <assert.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
tools/kvm/virtio/console.c +1 -2
Index: linux-2.6.git/tools/kvm/virtio/console.c
===================================================================
@@ -21,7 +21,6 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <termios.h>
-#include <assert.h>
 #include <unistd.h>
 #include <fcntl.h>
 

@@ -136,7 +135,7 @@ static int init_vq(struct kvm *kvm, void
 	struct virt_queue *queue;
 	void *p;
 
-	assert(vq < VIRTIO_CONSOLE_NUM_QUEUES);
+	BUG_ON(vq >= VIRTIO_CONSOLE_NUM_QUEUES);
 
 	compat__remove_message(compat_id);
tools/kvm/virtio/net.c +0 -1
Index: linux-2.6.git/tools/kvm/virtio/net.c
===================================================================
@@ -19,7 +19,6 @@
 #include <net/if.h>
 
 #include <unistd.h>
-#include <assert.h>
 #include <fcntl.h>
 
 #include <sys/socket.h>
tools/kvm/x86/bios.c +1 -1
Index: linux-2.6.git/tools/kvm/x86/bios.c
===================================================================
@@ -98,7 +98,7 @@ static void e820_setup(struct kvm *kvm)
 		};
 	}
 
-	BUILD_BUG_ON(i > E820_X_MAX);
+	BUG_ON(i > E820_X_MAX);
 
 	e820->nr_map = i;
 }
tools/kvm/x86/cpuid.c +0 -1
Index: linux-2.6.git/tools/kvm/x86/cpuid.c
===================================================================
@@ -5,7 +5,6 @@
 
 #include <sys/ioctl.h>
 #include <stdlib.h>
-#include <assert.h>
 
 #define CPUID_FUNC_PERFMON		0x0A
tools/kvm/x86/kvm.c +0 -1
Index: linux-2.6.git/tools/kvm/x86/kvm.c
===================================================================
@@ -15,7 +15,6 @@
 #include <sys/mman.h>
 #include <sys/stat.h>
 #include <stdbool.h>
-#include <assert.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
#6 lore raw
Quoted 6 lines · Cyrill Gorcunov
Quoted 3 lines

You don't need to convert all of them at the same time but we're not adding new assert() calls.

You both (Pekka and Ingo) know the way how to convince people ;) Something like below?

Perfect! I'll apply it later today. Thanks Cyrill!

#7 lore raw
In reply to Cyrill Gorcunov

tools/kvm/include/kvm/util.h extern void set_die_routine(void (*routi · line 56

Quoted 3 lines · Cyrill Gorcunov
tools/kvm/include/kvm/util.h +3 -0
+#
+#define BUILD_BUG_ON(condition)	((void)sizeof(char[1 - 2*!!(condition)]))
+#define BUG_ON(condition)	assert(!(condition))

Just a sidenote, the patch is fine but the above will result in weird double negated assertion messages.

it's better to just do our own __BUG() function and use it. That way it can also use the kernel standard 'BUG: ...' message format.

To make such BUG()s easier to debug via GDB, do this from the __BUG() handler:

	raise(SIGABRT);

GDB will catch that signal.

Thanks,

Ingo

author
#8 lore raw
Quoted 3 lines · Ingo Molnar

GDB will catch that signal.

Yeah, good point! Pekka, drop this patch please, I'll make new one at evening.

Cyrill

Acked-by
#9 lore raw
Quoted 6 lines · Cyrill Gorcunov
Quoted 3 lines · Ingo Molnar

GDB will catch that signal.

Yeah, good point! Pekka, drop this patch please, I'll make new one at evening.

Patch is good as-is IMO - assert() was used before so it's not a regression per se.

Acked-by Ingo Molnar <[email protected]>

The tool-specific BUG() implementation can be added as a delta on top of that. It's in fact better to keep those two steps separate.

Thanks,

Ingo

#10 lore raw
Quoted 6 lines · Cyrill Gorcunov
Quoted 6 lines · Ingo Molnar
Quoted 3 lines

GDB will catch that signal.

Yeah, good point! Pekka, drop this patch please, I'll make new one at evening.

Quoted 8 lines · Ingo Molnar

Patch is good as-is IMO - assert() was used before so it's not a regression per se.

Acked-by Ingo Molnar <[email protected]>

The tool-specific BUG() implementation can be added as a delta on top of that. It's in fact better to keep those two steps separate.

Agreed. Cyrill, please make it an incremental patch on top of this one.

author
#11 lore raw
In reply to Ingo Molnar

On Mon, Dec 19, 2011 at 11:50:31AM +0100, Ingo Molnar wrote:
...

Quoted 5 lines

The tool-specific BUG() implementation can be added as a delta on top of that. It's in fact better to keep those two steps separate.

OK, will do on top.

Cyrill