On Tue, 2014-01-14 at 08:47 +0200, Jack Morgenstein wrote:
> On Tue, 07 Jan 2014 14:01:18 +0100
> Paul Bolle <pebo...@tiscali.nl> wrote:
>
> > + } else {
> > + /* state == RES_CQ_HW */
> > + if (r->com.state != RES_CQ_ALLOCATED)
>
> if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED)
> to protect against any bad calls to this function
> (although I know that currently there are none).
So we end up with
} else if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED) {
err = -EINVAL;
} else {
err = 0;
}
don't we? Which is fine with me, as GCC still is then able to correctly
analyze this function.
> This also preserves the behavior of the switch statement.
>
> > err = -EINVAL;
> > - }
> > + else
> > + err = 0;
> > + }
> >
> > - if (!err) {
> > - r->com.from_state = r->com.state;
> > - r->com.to_state = state;
> > - r->com.state = RES_CQ_BUSY;
> > - if (cq)
> > - *cq = r;
> > - }
> > + if (!err) {
> > + r->com.from_state = r->com.state;
> > + r->com.to_state = state;
> > + r->com.state = RES_CQ_BUSY;
>
> Please keep the "if" here. Protects against (future) bad calls.
>
> > + *cq = r;
> > }
There seems to be a school of thought that says it's better to trigger
an Oops if a programming error is made (in this case by passing a NULL
cq) then silently handle that (future) programming error and make
debugging harder. But, even it that school of thought really exists,
this is up to you. Besides, it's only a triviality I added to my
patches.
Thanks for the review! I hope to send in a v2 of my patches shortly.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
