irda: fix type of struct irda_ias_set.attribute.irda_attrib_string.len

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 =====

@@ -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;

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

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

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
---

278c278
< CFLAGS := -Wall -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 <[email protected]> 1.650.933.1373

Yes ! You are 100% correct, changing this would break all the
IrDA user space, which I'm not keen on doing.

Yes, in this case the test should be removed with a comment.

Jean