[PATCH] irda: fix type of struct irda_ias_set.attribute.irda_attrib_string.len

[PATCH] irda: fix type of struct irda_ias_set.attribute.irda_attrib_string.len

[Arnaldo Carvalho de Melo]

  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;

Re: [PATCH] irda: fix type of struct irda_ias_set.attribute.irda_attrib_string.len

[Linus Torvalds]

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

Re: [PATCH] irda: fix type of struct irda_ias_set.attribute.irda_attrib_string.len

[Linus Torvalds]

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

Re: [PATCH] irda: fix type of struct irda_ias_set.attribute.irda_attrib_string.len

[Paul Jackson]

A couple of months ago, Linus wrote:
> 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.

Then get rid of that warning, using -Wno-sign-compare:

The following change to the top level Makefile tells gcc not to complain
about this:

===== Makefile 1.439 vs edited =====
164c164
< HOSTCFLAGS    = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer
---
> HOSTCFLAGS    = -Wall -Wno-sign-compare -Wstrict-prototypes -O2 -fomit-frame-pointer
278c278
< CFLAGS                := -Wall -Wstrict-prototypes -Wno-trigraphs -O2 \
---
> CFLAGS                := -Wall -Wno-sign-compare -Wstrict-prototypes -Wno-trigraphs -O2 \

Would you like me to send Andrew a real patch for this?

I have been running all my personal builds with this change for a month
now, ever since I picked up some version of gcc that is fond of that
warning.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] irda: fix type of struct irda_ias_set.attribute.irda_attrib_string.len

[Jean Tourrilhes]

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