On Wed, May 15, 2019 at 08:43:33PM +0200, Bart Van Assche wrote:
> On 5/15/19 8:33 PM, Leon Romanovsky wrote:
> > On Wed, May 15, 2019 at 07:32:35PM +0200, Bart Van Assche wrote:
> >> On 5/15/19 7:06 PM, Leon Romanovsky wrote:
> >>> + list_for_each_entry_safe(host, tmp_host, &srp_dev->dev_list, list) {
> >>> + char name[IB_DEVICE_NAME_MAX * 2] = {};
> >> ^^^^^^^^^^^^^^^^^^^^^^
> >> I think this should be IB_DEVICE_NAME_MAX + 8 instead of ... * 2. Please
> >> also consider to leave out the initialization of the char array since
> >> snprintf() overwrites the array whether or not it has been initialized.
> >
> > Any reason why shoud we care for this micro optimizations?
>
> Good kernel developers care about writing code that is not confusing to
> the reader. IB_DEVICE_NAME_MAX * 2 is confusing because the generated
> string is not built of two device names. The "= {}" part is confusing
> because immediately after that initialization the entire string gets
> overwritten. This makes the reader wonder: what does that initialization
> do there?
>
> >>> + snprintf(name, IB_DEVICE_NAME_MAX * 2, "srp-%s-%d",
> >> ^^^^^^^^^^^^^^^^^^^^^^
> >> Please change this into sizeof(name) such that the size expression only
> >> occurs once.
> >
> > The same as sizeof it is calculated once at the stage of defines
> > expansion.
>
> I think that snprintf(buf, sizeof(buf), ...) is much more common in the
> kernel than snprintf(buf, size_expression, ...).
No problem, you think that it is important, I'll change.
Thanks for the review.
>
> Thanks,
>
> Bart.
>
