* [PATCH] kvm tools: Use assert() helper to check a variable value @ 2011-12-18 21:35 Cyrill Gorcunov 2011-12-19 7:13 ` Pekka Enberg 0 siblings, 1 reply; 12+ messages in thread From: Cyrill Gorcunov @ 2011-12-18 21:35 UTC (permalink / raw) To: Pekka Enberg; +Cc: Sasha Levin, Asias He, Ingo Molnar, Thomas Gleixner, KVM-ML 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; } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-18 21:35 [PATCH] kvm tools: Use assert() helper to check a variable value Cyrill Gorcunov @ 2011-12-19 7:13 ` Pekka Enberg 2011-12-19 7:57 ` Cyrill Gorcunov 0 siblings, 1 reply; 12+ messages in thread From: Pekka Enberg @ 2011-12-19 7:13 UTC (permalink / raw) To: Cyrill Gorcunov Cc: Sasha Levin, Asias He, Ingo Molnar, Thomas Gleixner, KVM-ML 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-19 7:13 ` Pekka Enberg @ 2011-12-19 7:57 ` Cyrill Gorcunov 2011-12-19 8:04 ` Ingo Molnar 2011-12-19 8:19 ` Pekka Enberg 0 siblings, 2 replies; 12+ messages in thread From: Cyrill Gorcunov @ 2011-12-19 7:57 UTC (permalink / raw) To: Pekka Enberg; +Cc: Sasha Levin, Asias He, Ingo Molnar, Thomas Gleixner, KVM-ML 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-19 7:57 ` Cyrill Gorcunov @ 2011-12-19 8:04 ` Ingo Molnar 2011-12-19 8:19 ` Pekka Enberg 1 sibling, 0 replies; 12+ messages in thread From: Ingo Molnar @ 2011-12-19 8:04 UTC (permalink / raw) To: Cyrill Gorcunov Cc: Pekka Enberg, Sasha Levin, Asias He, Thomas Gleixner, KVM-ML * 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-19 7:57 ` Cyrill Gorcunov 2011-12-19 8:04 ` Ingo Molnar @ 2011-12-19 8:19 ` Pekka Enberg 2011-12-19 8:25 ` Cyrill Gorcunov 1 sibling, 1 reply; 12+ messages in thread From: Pekka Enberg @ 2011-12-19 8:19 UTC (permalink / raw) To: Cyrill Gorcunov Cc: Sasha Levin, Asias He, Ingo Molnar, Thomas Gleixner, KVM-ML 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-19 8:19 ` Pekka Enberg @ 2011-12-19 8:25 ` Cyrill Gorcunov 2011-12-19 8:48 ` Pekka Enberg 2011-12-19 10:40 ` Ingo Molnar 0 siblings, 2 replies; 12+ messages in thread From: Cyrill Gorcunov @ 2011-12-19 8:25 UTC (permalink / raw) To: Pekka Enberg; +Cc: Sasha Levin, Asias He, Ingo Molnar, Thomas Gleixner, KVM-ML 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-19 8:25 ` Cyrill Gorcunov @ 2011-12-19 8:48 ` Pekka Enberg 2011-12-19 10:40 ` Ingo Molnar 1 sibling, 0 replies; 12+ messages in thread From: Pekka Enberg @ 2011-12-19 8:48 UTC (permalink / raw) To: Cyrill Gorcunov Cc: Sasha Levin, Asias He, Ingo Molnar, Thomas Gleixner, KVM-ML 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! ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-19 8:25 ` Cyrill Gorcunov 2011-12-19 8:48 ` Pekka Enberg @ 2011-12-19 10:40 ` Ingo Molnar 2011-12-19 10:49 ` Cyrill Gorcunov 1 sibling, 1 reply; 12+ messages in thread From: Ingo Molnar @ 2011-12-19 10:40 UTC (permalink / raw) To: Cyrill Gorcunov Cc: Pekka Enberg, Sasha Levin, Asias He, Thomas Gleixner, KVM-ML * 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-19 10:40 ` Ingo Molnar @ 2011-12-19 10:49 ` Cyrill Gorcunov 2011-12-19 10:50 ` Ingo Molnar 0 siblings, 1 reply; 12+ messages in thread From: Cyrill Gorcunov @ 2011-12-19 10:49 UTC (permalink / raw) To: Ingo Molnar; +Cc: Pekka Enberg, Sasha Levin, Asias He, Thomas Gleixner, KVM-ML 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-19 10:49 ` Cyrill Gorcunov @ 2011-12-19 10:50 ` Ingo Molnar 2011-12-19 10:57 ` Pekka Enberg 2011-12-19 10:57 ` Cyrill Gorcunov 0 siblings, 2 replies; 12+ messages in thread From: Ingo Molnar @ 2011-12-19 10:50 UTC (permalink / raw) To: Cyrill Gorcunov Cc: Pekka Enberg, Sasha Levin, Asias He, Thomas Gleixner, KVM-ML * 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-19 10:50 ` Ingo Molnar @ 2011-12-19 10:57 ` Pekka Enberg 2011-12-19 10:57 ` Cyrill Gorcunov 1 sibling, 0 replies; 12+ messages in thread From: Pekka Enberg @ 2011-12-19 10:57 UTC (permalink / raw) To: Ingo Molnar Cc: Cyrill Gorcunov, Sasha Levin, Asias He, Thomas Gleixner, KVM-ML * 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kvm tools: Use assert() helper to check a variable value 2011-12-19 10:50 ` Ingo Molnar 2011-12-19 10:57 ` Pekka Enberg @ 2011-12-19 10:57 ` Cyrill Gorcunov 1 sibling, 0 replies; 12+ messages in thread From: Cyrill Gorcunov @ 2011-12-19 10:57 UTC (permalink / raw) To: Ingo Molnar; +Cc: Pekka Enberg, Sasha Levin, Asias He, Thomas Gleixner, KVM-ML 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-12-19 10:58 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-18 21:35 [PATCH] kvm tools: Use assert() helper to check a variable value Cyrill Gorcunov 2011-12-19 7:13 ` Pekka Enberg 2011-12-19 7:57 ` Cyrill Gorcunov 2011-12-19 8:04 ` Ingo Molnar 2011-12-19 8:19 ` Pekka Enberg 2011-12-19 8:25 ` Cyrill Gorcunov 2011-12-19 8:48 ` Pekka Enberg 2011-12-19 10:40 ` Ingo Molnar 2011-12-19 10:49 ` Cyrill Gorcunov 2011-12-19 10:50 ` Ingo Molnar 2011-12-19 10:57 ` Pekka Enberg 2011-12-19 10:57 ` Cyrill Gorcunov
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).