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

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

[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 <gorcunov@gmail.com>
---
 tools/kvm/x86/bios.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.git/tools/kvm/x86/bios.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/x86/bios.c
+++ 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;
 }

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Pekka Enberg]

On Mon, 19 Dec 2011, Cyrill Gorcunov wrote:
> BUILD_BUG_ON is unable to catch errors on expression which
> can't be evaluated at compile time.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> tools/kvm/x86/bios.c |    3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.git/tools/kvm/x86/bios.c
> ===================================================================
> --- linux-2.6.git.orig/tools/kvm/x86/bios.c
> +++ 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);

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

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Cyrill Gorcunov]

On Mon, Dec 19, 2011 at 09:13:28AM +0200, Pekka Enberg wrote:
> >
> >-	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

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On Mon, Dec 19, 2011 at 09:13:28AM +0200, Pekka Enberg wrote:
> > >
> > >-	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

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Pekka Enberg]

On Mon, Dec 19, 2011 at 09:13:28AM +0200, Pekka Enberg wrote:
>> >
>> >-    BUILD_BUG_ON(i > E820_X_MAX);
>> >+    assert(i <= E820_X_MAX);
>>
>> We should use BUG_ON() like tools/perf does.

On Mon, Dec 19, 2011 at 9:57 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 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.

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Cyrill Gorcunov]

On Mon, Dec 19, 2011 at 10:19:43AM +0200, Pekka Enberg wrote:
> On Mon, Dec 19, 2011 at 09:13:28AM +0200, Pekka Enberg wrote:
> >> >
> >> >-    BUILD_BUG_ON(i > E820_X_MAX);
> >> >+    assert(i <= E820_X_MAX);
> >>
> >> We should use BUG_ON() like tools/perf does.
> 
> On Mon, Dec 19, 2011 at 9:57 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > 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 <gorcunov@gmail.com>
---
 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(-)

Index: linux-2.6.git/tools/kvm/include/kvm/util.h
===================================================================
--- linux-2.6.git.orig/tools/kvm/include/kvm/util.h
+++ 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 {								\
Index: linux-2.6.git/tools/kvm/ioport.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/ioport.c
+++ 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>
Index: linux-2.6.git/tools/kvm/kvm-cmd.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/kvm-cmd.c
+++ 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;
 	}
Index: linux-2.6.git/tools/kvm/kvm.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/kvm.c
+++ 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>
Index: linux-2.6.git/tools/kvm/pci.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/pci.c
+++ 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];
 }
 
Index: linux-2.6.git/tools/kvm/powerpc/kvm.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/powerpc/kvm.c
+++ 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>
Index: linux-2.6.git/tools/kvm/virtio/console.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/virtio/console.c
+++ 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);
 
Index: linux-2.6.git/tools/kvm/virtio/net.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/virtio/net.c
+++ 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>
Index: linux-2.6.git/tools/kvm/x86/bios.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/x86/bios.c
+++ 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;
 }
Index: linux-2.6.git/tools/kvm/x86/cpuid.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/x86/cpuid.c
+++ 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
 
Index: linux-2.6.git/tools/kvm/x86/kvm.c
===================================================================
--- linux-2.6.git.orig/tools/kvm/x86/kvm.c
+++ 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>

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Pekka Enberg]

On Mon, Dec 19, 2011 at 10:25 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> 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!

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> +#
> +#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

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Cyrill Gorcunov]

On Mon, Dec 19, 2011 at 11:40:09AM +0100, Ingo Molnar wrote:
> 
> GDB will catch that signal.
> 

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

	Cyrill

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On Mon, Dec 19, 2011 at 11:40:09AM +0100, Ingo Molnar wrote:
> > 
> > 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 <mingo@elte.hu>

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

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Pekka Enberg]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> On Mon, Dec 19, 2011 at 11:40:09AM +0100, Ingo Molnar wrote:
>>>
>>> GDB will catch that signal.
>>>
>>
>> Yeah, good point! Pekka, drop this patch please, I'll make new one at evening.

On Mon, 19 Dec 2011, Ingo Molnar wrote:
> Patch is good as-is IMO - assert() was used before so it's not a
> regression per se.
>
> Acked-by: Ingo Molnar <mingo@elte.hu>
>
> 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.

Re: [PATCH] kvm tools: Use assert() helper to check a variable value

[Cyrill Gorcunov]

On Mon, Dec 19, 2011 at 11:50:31AM +0100, Ingo Molnar wrote:
...
> 
> 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