On Fri, Jan 12, 2018 at 5:09 PM, Bart Van Assche <Bart.VanAssche@xxxxxxx> wrote: > On Fri, 2018-01-12 at 15:45 +0200, Andy Shevchenko wrote: >> On Fri, Jan 12, 2018 at 1:46 PM, Wang <wangxiongfeng2@xxxxxxxxxx> wrote: >> This one is false positive. >> > + strlcpy (karg.name, ioc->name, MPT_MAX_NAME); >> > + strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH); >> >> These two as well. > > But using strlcpy() makes the code easier to read. This is another story, not mentioned in the original commit message. > Xiongfeng, if you want to continue with this patch, please change the third > argument of all strlcpy() calls into a sizeof() expression. That make the > code easier to verify. > >> > - strncpy(karg.serial_number, " ", 24); >> > + strlcpy(karg.serial_number, " ", 24); >> >> This one is interesting indeed. >> Though the fix would be rather something like >> >> memset(&karg.serial_number, " ", 24); // leave 24 for best performance of memset >> karg.serial_number[24-1] = '\0'; > > Does performance really matter in this context? Nope, but still good to understand this aspect. On some architectures unaligned access is expensive. > How about the following instead: > > snprintf(karg.serial_number, sizeof(karg.serial_number), "%*s", (int)(karg.serial_number) - 1, ""); ...and you is talking about "easy to read"?! So, my view on the patch is: 1) fix a real issue w/o touching everything around; 2) (optionally) move to strlcpy() with a correct selling point. -- With Best Regards, Andy Shevchenko
