Hi Igor,
On 8/12/21 10:24 PM, Igor Zhbanov wrote:
> Hi Simon,
>
> ср, 28 июл. 2021 г. в 13:19, THOBY Simon <Simon.THOBY@xxxxxxxxxx>:
>> Nit: considering that allowed_caps_hex is only used in this function, and that it represents a small amount of
>> stack space, couldn't you define it directly in the function?
>>
>>> + len = strlen(allowed_caps_hex);
>
> It is also used as sysctl parameter input buffer in nax_sysctl_table[]
> for "allowed_caps" parameter.
>
Yes, what I meant was that maybe you could just declare it at the beginning of the function,
and not use it at all in the sysctl table. Because as I see it, you only use allowed_caps_hex in the sysctl
table to copy the string to that temporary (variable), and its use is limited to that one function.
Instead of:
+ if ((error = proc_dostring(table, write, buffer, lenp, ppos)))
+ return error;
You could probably get away with something like:
> +static int
> +nax_dostring(struct ctl_table *table, int write, void *buffer,
> + size_t *lenp, loff_t *ppos)
> +{
+ int error;
+ char allowed_caps_hex[ALLOWED_CAPS_HEX_LEN + 1];
[...]
+ len = strlen(allowed_caps_hex);
+ if (len > ALLOWED_CAPS_HEX_LEN)
+ return -EINVAL;
+ strncpy(allowed_caps_hex, buffer, ALLOWED_CAPS_HEX_LEN + 1);
I have to admit that having nearly no kernel development experience, I'm not
sure there is any guidance on the matter (maybe there is even guidance that recommend
preferring what your original patch does to what I suggested) but I think it is
unnecessary to have a variable accessible to the whole file but being used to hold (small)
temporary data for a single function.
In any case, I would appreciate comments for other reviewers if what I'm saying is completely
wrong, so as to not mislead you in doing adverse changes.
Thanks,
Simon
