CC [M] net/irda/af_irda.o
net/irda/af_irda.c: In function `irda_setsockopt':
net/irda/af_irda.c:1894: warning: comparison is always false due to limited range of data type
in irda_setsockopt:
/* Should check charset & co */
/* Check length */
if(ias_opt->attribute.irda_attrib_string.len >
IAS_MAX_STRING) {
kfree(ias_opt);
return -EINVAL;
}
Ok, ias_opt->attribute.irda_attrib_string.len is __u8, but
IAS_MAX_STRING = 256... so attribute.irda_attrib_string.len has to be at least
__u18, this patch fix this, please see if it is appropriate and if it is so,
apply.
Best Regards,
- Arnaldo
===== include/linux/irda.h 1.7 vs edited =====
--- 1.7/include/linux/irda.h Wed Jun 4 11:16:33 2003
+++ edited/include/linux/irda.h Mon Nov 10 23:56:33 2003
@@ -151,7 +151,7 @@
__u8 octet_seq[IAS_MAX_OCTET_STRING];
} irda_attrib_octet_seq;
struct {
- __u8 len;
+ __u16 len;
__u8 charset;
__u8 string[IAS_MAX_STRING];
} irda_attrib_string;
On Tue, 11 Nov 2003, Arnaldo Carvalho de Melo wrote:
>
> Ok, ias_opt->attribute.irda_attrib_string.len is __u8, but
> IAS_MAX_STRING = 256... so attribute.irda_attrib_string.len has to be at least
> __u18, this patch fix this, please see if it is appropriate and if it is so,
> apply.
No, please don't. This
(a) changes ABI structures that are exported to user space
(b) is unnecessary - since the problem is the _warning_, not the test.
Just shut the warning up by either removing the test (replacing it with a
comment about why it's unnecessary), or by adding a cast, ie
unsigned int len = ias_opt->attribute.irda_attrib_string.len;
if (len > IAS_MAX_STRING) {
...
in case the code ever expects IAS_MAX_STRING to be shorter (or if the type
is ever changed).
Linus
On Mon, 10 Nov 2003, Linus Torvalds wrote:
>
> No, please don't.
Btw, this is a general thing with warnings that the compiler spits out.
Some compiler warnings are for perfectly ok code.
Sometimes the warning itself is fundamentally broken (the sign warnings
that gcc used to spit out), sometimes it's because the code is 100% right
for some abstract reason that the compiler can't figure out.
An example of such an "abstract reason" is exactly something like this
case, where the user really didn't _care_ too deeply about the type he was
checking, and was caring a lot more about a totally _independent_ sanity
check, which just happened to be unrelated to the type size. In this case,
having the compiler just silently notice that "oh, this can't happen,
because I know the range in this case" is ok.
And if the code is right, just re-write it in a form that avoids the
warning. So
if (a = b) {
should become
a = b;
if (a) {
because it's clearer _and_ avoids the warning (don't just blindly add a
parenthesis).
That's why I hate the "sign compare" warning of gcc so much - it warns
about things that you CANNOT sanely write in any other way. That makes
that particular warning _evil_, since it encourages people to write crap
code.
In this case, the warning is easily avoided by splitting the code up a
bit, and accepting an unnecessary cast (which hopefully the compiler may
eventually notice it unnecessary). And as the warning in general is good,
that's fine - the warning does not generally _encourage_ bad programming.
Linus
On Mon, Nov 10, 2003 at 06:59:59PM -0800, Linus Torvalds wrote:
>
> On Tue, 11 Nov 2003, Arnaldo Carvalho de Melo wrote:
> >
> > Ok, ias_opt->attribute.irda_attrib_string.len is __u8, but
> > IAS_MAX_STRING = 256... so attribute.irda_attrib_string.len has to be at least
> > __u18, this patch fix this, please see if it is appropriate and if it is so,
> > apply.
>
> No, please don't. This
> (a) changes ABI structures that are exported to user space
Yes ! You are 100% correct, changing this would break all the
IrDA user space, which I'm not keen on doing.
> (b) is unnecessary - since the problem is the _warning_, not the test.
>
> Just shut the warning up by either removing the test (replacing it with a
> comment about why it's unnecessary), or by adding a cast, ie
>
> unsigned int len = ias_opt->attribute.irda_attrib_string.len;
>
> if (len > IAS_MAX_STRING) {
> ...
>
> in case the code ever expects IAS_MAX_STRING to be shorter (or if the type
> is ever changed).
Yes, in this case the test should be removed with a comment.
> Linus
Jean