[patch] trace: cleanup: make some types unsigned

[patch] trace: cleanup: make some types unsigned

[Dan Carpenter]

The problem here is that I'm trying to silence a static checker
warning.  In replace_preds() we cap n_preds at MAX_FILTER_PRED but
we don't check for negative values.  It can't actually be negative
values, but the static checkers get confused.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 256764e..7f4daff 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -790,7 +790,7 @@ static struct event_filter *__alloc_filter(void)
 	return filter;
 }
 
-static int __alloc_preds(struct event_filter *filter, int n_preds)
+static int __alloc_preds(struct event_filter *filter, unsigned int n_preds)
 {
 	struct filter_pred *pred;
 	int i;
@@ -1361,10 +1361,10 @@ static int check_preds(struct filter_parse_state *ps)
 	return 0;
 }
 
-static int count_preds(struct filter_parse_state *ps)
+static unsigned int count_preds(struct filter_parse_state *ps)
 {
 	struct postfix_elt *elt;
-	int n_preds = 0;
+	unsigned int n_preds = 0;
 
 	list_for_each_entry(elt, &ps->postfix, list) {
 		if (elt->op == OP_NONE)
@@ -1601,7 +1601,7 @@ static int replace_preds(struct ftrace_event_call *call,
 	struct postfix_elt *elt;
 	struct pred_stack stack = { }; /* init to NULL */
 	int err;
-	int n_preds = 0;
+	unsigned int n_preds = 0;
 
 	n_preds = count_preds(ps);
 	if (n_preds >= MAX_FILTER_PRED) {

[patch] trace: cleanup: make some types unsigned

[Dan Carpenter]

The problem here is that I'm trying to silence a static checker
warning.  In replace_preds() we cap n_preds at MAX_FILTER_PRED but
we don't check for negative values.  It can't actually be negative
values, but the static checkers get confused.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 256764e..7f4daff 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -790,7 +790,7 @@ static struct event_filter *__alloc_filter(void)
 	return filter;
 }
 
-static int __alloc_preds(struct event_filter *filter, int n_preds)
+static int __alloc_preds(struct event_filter *filter, unsigned int n_preds)
 {
 	struct filter_pred *pred;
 	int i;
@@ -1361,10 +1361,10 @@ static int check_preds(struct filter_parse_state *ps)
 	return 0;
 }
 
-static int count_preds(struct filter_parse_state *ps)
+static unsigned int count_preds(struct filter_parse_state *ps)
 {
 	struct postfix_elt *elt;
-	int n_preds = 0;
+	unsigned int n_preds = 0;
 
 	list_for_each_entry(elt, &ps->postfix, list) {
 		if (elt->op = OP_NONE)
@@ -1601,7 +1601,7 @@ static int replace_preds(struct ftrace_event_call *call,
 	struct postfix_elt *elt;
 	struct pred_stack stack = { }; /* init to NULL */
 	int err;
-	int n_preds = 0;
+	unsigned int n_preds = 0;
 
 	n_preds = count_preds(ps);
 	if (n_preds >= MAX_FILTER_PRED) {

Re: [patch] trace: cleanup: make some types unsigned

[Steven Rostedt]

On Fri, 2011-10-07 at 16:27 +0300, Dan Carpenter wrote:
> The problem here is that I'm trying to silence a static checker
> warning.  In replace_preds() we cap n_preds at MAX_FILTER_PRED but
> we don't check for negative values.  It can't actually be negative
> values, but the static checkers get confused.

I really hate to uglify code for the sake of static checkers.

This code may change in the near future, and the possibility that
n_preds may become a possibility. Perhaps we should add a:

WARN_ON(n_preds < 0);

If in the future the count_preds() changes and incorrectly produces a
negative number, or perhaps even overflows int, we wont catch it with
unsigned.

-- Steve

Re: [patch] trace: cleanup: make some types unsigned

[Steven Rostedt]

On Fri, 2011-10-07 at 16:27 +0300, Dan Carpenter wrote:
> The problem here is that I'm trying to silence a static checker
> warning.  In replace_preds() we cap n_preds at MAX_FILTER_PRED but
> we don't check for negative values.  It can't actually be negative
> values, but the static checkers get confused.

I really hate to uglify code for the sake of static checkers.

This code may change in the near future, and the possibility that
n_preds may become a possibility. Perhaps we should add a:

WARN_ON(n_preds < 0);

If in the future the count_preds() changes and incorrectly produces a
negative number, or perhaps even overflows int, we wont catch it with
unsigned.

-- Steve

Re: [patch] trace: cleanup: make some types unsigned

[Dan Carpenter]

On Fri, Oct 07, 2011 at 09:38:51AM -0400, Steven Rostedt wrote:
> On Fri, 2011-10-07 at 16:27 +0300, Dan Carpenter wrote:
> > The problem here is that I'm trying to silence a static checker
> > warning.  In replace_preds() we cap n_preds at MAX_FILTER_PRED but
> > we don't check for negative values.  It can't actually be negative
> > values, but the static checkers get confused.
> 
> I really hate to uglify code for the sake of static checkers.
> 
> This code may change in the near future, and the possibility that
> n_preds may become a possibility. Perhaps we should add a:
> 
> WARN_ON(n_preds < 0);
> 
> If in the future the count_preds() changes and incorrectly produces a
> negative number, or perhaps even overflows int, we wont catch it with
> unsigned.

I've sent a couple type changes to silence static checker warnings,
but I haven't been pushing it, because I'm interested to see what
people think about them first.  I didn't think unsigned int was
particularly ugly, but now that you point it out I guess it is
needlessly pedantic and longer to type.  So it's fine if you ignore
the patch.

Please don't add the WARN_ON().  WARN_ON()s are uglier than unsigned
ints.  WARN_ON() don't solve any problems, they just make debugging
the crash easier.  Are we going to crash here, and if so, do we
expect that debugging it will be difficult?  Probably not.

In theory, static checkers should be able to look at this code and
know that n_preds can't overflow.  So yeah.  Let's call this a
static checker bug and move on.

regards,
dan carpenter

Re: [patch] trace: cleanup: make some types unsigned

[Dan Carpenter]

On Fri, Oct 07, 2011 at 09:38:51AM -0400, Steven Rostedt wrote:
> On Fri, 2011-10-07 at 16:27 +0300, Dan Carpenter wrote:
> > The problem here is that I'm trying to silence a static checker
> > warning.  In replace_preds() we cap n_preds at MAX_FILTER_PRED but
> > we don't check for negative values.  It can't actually be negative
> > values, but the static checkers get confused.
> 
> I really hate to uglify code for the sake of static checkers.
> 
> This code may change in the near future, and the possibility that
> n_preds may become a possibility. Perhaps we should add a:
> 
> WARN_ON(n_preds < 0);
> 
> If in the future the count_preds() changes and incorrectly produces a
> negative number, or perhaps even overflows int, we wont catch it with
> unsigned.

I've sent a couple type changes to silence static checker warnings,
but I haven't been pushing it, because I'm interested to see what
people think about them first.  I didn't think unsigned int was
particularly ugly, but now that you point it out I guess it is
needlessly pedantic and longer to type.  So it's fine if you ignore
the patch.

Please don't add the WARN_ON().  WARN_ON()s are uglier than unsigned
ints.  WARN_ON() don't solve any problems, they just make debugging
the crash easier.  Are we going to crash here, and if so, do we
expect that debugging it will be difficult?  Probably not.

In theory, static checkers should be able to look at this code and
know that n_preds can't overflow.  So yeah.  Let's call this a
static checker bug and move on.

regards,
dan carpenter