On Wed, Nov 13, 2019 at 08:39:13PM +0300, Dan Carpenter wrote:
> On Mon, Nov 11, 2019 at 11:21:15PM +0000, John Levon wrote:
> >
> > Given:
> >
> > enum ib_qp_state {
> > IB_QPS_RESET,
> > IB_QPS_INIT,
> > IB_QPS_RTR,
> > IB_QPS_RTS,
> > IB_QPS_SQD,
> > IB_QPS_SQE,
> > IB_QPS_ERR
> > };
> >
> > int p(enum ib_qp_state cur_state, enum ib_qp_state next_state)
> > {
> > #if 0
> > if (cur_state < 0 || cur_state > IB_QPS_ERR ||
> > next_state < 0 || next_state > IB_QPS_ERR) {
> > return (5);
> > }
> > #else
> > if (next_state < 0 || next_state > IB_QPS_ERR) {
> > return (5);
> > }
> > #endif
> >
> > return (0);
> > }
> >
> > current smatch is happy. But "#if 1" above instead, and:
> >
> > jlevon@sent:~/src/smatch$ ./smatch a.c
> > ./smatch: a.c:15 p() warn: unsigned 'cur_state' is never less than zero.
> >
> > Why is "next_state" not reported equally?
>
> My plan was that neither one would print a warning. When people do
>
> if (foo < 0 || foo > max)
>
> then normally it's not a real life bug... Linus said at kernel summit
> that he doesn't like when people send patches for those.
Right, in fact the code this comes from is similar: it's a
belt-and-braces check that we didn't somehow get a bad state value.
There's definitely other cases where this might be useful, though I
wouldn't object to disabling it by default altogether.
thanks
john
