On Mon, Oct 11, 2010 at 06:40:38PM +0200, Dan Carpenter wrote: > On Mon, Oct 11, 2010 at 11:40:09AM +0100, Mark Brown wrote: > > I'm not going to apply this. snprintf() returns a signed type, checking > > that the return value is a reasonable thing to do here - at worst we're > > wasting a few cycles in code that's nowhere near a hot path, at best > > we're robust in the face of a decision to add error reporting to > > snprintf() so it's hard to see this change as an improvement. > It's not a matter of cycles. My check probably takes the exact same > number of cycles as your check. The point is that checking for negative > doesn't make any sort of sense. If we did return an error code then we Remember that we also have to be able to read the code; it's probably safe to assume that not everyone has the quirks of every snprintf() implementation committed to memory. Except for the microoptimisation all you're doing here is making the code harder to read. > If "PAGE_SIZE - ret" is a negative number then it triggers a > WARN_ON_ONCE() in vsnprintf(). It's not possible in this case however. > Really the original code works fine in practice because the information > we are printing out is less than PAGE_SIZE. In actual fact quite a few devices have enough registers to be truncated, meaning that it's not only possible but likely we'll exercise the cases that deal with the end of buffer. If snprintf() is returning values larger than buffer size it was given we're likely to have an issue but it seems that there's something missing in your analysis since we're never seeing WARN_ON()s and are instead seeing the behaviour the code is intended to give, which is to truncate the output when we run out of space. Could you re-check your analysis, please? -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
