On 05/15/2013 08:06 PM, Dan Carpenter wrote: > Please read my first email again. Pretty much everything I wanted > to say is there. > > http://driverdev.linuxdriverproject.org/pipermail/devel/2013-May/038083.html > Oh, sorry, it is my fault. Originally, I only read the upper half of the mail, did not see the bottom half. I should notice next time. And that mail is really complete. > On Wed, May 15, 2013 at 06:09:14PM +0800, Chen Gang wrote: >> > If the type of 'probe_rsp->rawData[]' is changed to 'char', it will >> > compare between 'char' and 'int', in this condition, casting to 'int' is >> > the normal way, all things are OK. > You mean "unsigned char" instead of "char"... Signed char would be > a bug. > >> > >> > If use min_t(u8..., we have to look up the 'HCF_MAX_NAME_LEN' whether is >> > changed upper than 255. > No. > 1) I would assume the programmer had considered that. > 2) That kind of a bug would almost certainly be caught in testing. > 3) How many values with a name like HCF_MAX_NAME_LEN are higher than > 255? Names are normally are 16-32 characters long. > > When I am reviewing and I see min_t(): > 1) I assume that the two types are different and that the compiler > warned about it. > 2) I assume that HCF_MAX_NAME_LEN is an int so > "HCF_MAX_NAME_LEN - 1" is also an int. > > When I see "min_t(int, XXX, HCF_MAX_NAME_LEN - 1)" then I think: > > 1) I know that "HCF_MAX_NAME_LEN - 1" is int, but I don't know the > type for "XXX". > 2) Are negative values for XXX valid? > 3) Did the author stop to consider that high values can be cast to > negative? > 4) A lot of people just use "int" without even considering if it's > appropriate. For example, here is a real bug I fixed last week: > http://marc.info/?l=linux-kernel&m=136845723902257&w=2 > This bug can only be triggered by root but it was still a real > bug where "int" was used instead of "unsigned int". > > If I see that the type is cast using "min_t(u8," then I will assume > that "XXX" is a u8. > > It is also possible that the author chose "min_t(u8," when neither > side of the comparison is a u8. The common reason for that is > because they are assigning the result to an u8 variable. But from > the immediate context I would see that that is not the case here. > After read the bottom half of your that mail, we really need not talk the details again. > But again, in your specific cast casting it to an int is fine. It > doesn't cause a bug. It only takes a little extra time for a > reviewer. > I will send min_t(u8..., in our case, it is better than min_t(int.... > Whatever... It's up to you. Send any patch which applies and has > the proper signed-off-by and I will ack it. :P > Oh, it is my fault, I provide the incorrect signed-off-by, I make this patch in another machine which forgot to add the default signed-off-by. I should notice next time. And it seems the patch sent too late, the original patch has already applied into next tree, so I will send the fix up patch for it. Thanks. -- Chen Gang Asianux Corporation _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
