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, ...).
Thanks,
Bart.
