On 04/14/2014 10:22 AM, David Binderman wrote: > Hello there, > > ---------------------------------------- >> But my point remains to the original poster: a patch >> without justification is unlikely to be applied. Document WHY you think >> the existing code is a bug, not just HOW to fix it, for your patch to be >> usefully considered. > > Standard software engineering practice is to look before leaping. > This means always check the array index before use. > The static analyser implements that standard practice.
Therefore, the static analyzer (_which_ static analyzer? you didn't state that) has exposed a false positive, and the WHY for the patch is to silence the false positive of the static analyzer. But have you also filed a bug against the analyzer about its false positive? > > The code in question, independent of whether it works ok or not, > does it's work in a non-standard way when the standard way > is easy to achieve and has some possible benefits for robustness, > as well as being easier on the eye to the experienced code reviewer. Indeed, reading JUST the code present in this thread, I did not realize that invokers[] was 1. always NULL-terminated, and 2. possibly longer than 5 elements; I also did not see that this loop wants to stop at the first instance of either end-of-list or capped at 5 (stopping early on an array is indeed an unusual construct). Reading the code in full context, I now see that, which is why I conclude that the static analyzer reported a false positive. Had you provided your patch with context lines, rather than just the one line being modified, it might have also saved some reviewer time. > > Anyone experienced looking at the code will always need to examine it > more closely to find out why it's a good idea in this case to use an array > index and *then* sanity check it's value. So use that as the justification for the patch, rather than expecting us to figure it out your intentions on our own. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
