Thanks Hans,
On Tue, Sep 5, 2023, at 7:17 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 8/29/23 15:15, Mark Pearson wrote:
<snip>
>> +
>> + /* Check if user is trying to change the save mode */
>> + if (!strncmp(buf, "bulk", 4) || !strncmp(buf, "single", 6)) {
>> + tlmi_priv.save_mode = strncmp(buf, "bulk", 4) ? TLMI_SAVE_SINGLE : TLMI_SAVE_BULK;
>> + return count;
>> + }
>> + if (strncmp(buf, "save", 4))
>> + return -EINVAL;
>
> Things look good up to this point, but I'm not happy
> with the string parsing here. Using strncmp to avoid
> a possible '\n' means that writing
> "bulk extra special with onions" will also match "bulk".
>
> Instead I suggest the following (better names
> for the enum are welcome):
>
> enum { save_single, save_bulk, save_save };
>
> const char * const save_strings[] = {
> [save_single] = "single",
> [save_bulk] = "bulk",
> [save_save] = "save",
> };
>
> int ret = 0;
> int cmd;
>
> cmd = sysfs_match_string(save_strings, buf);
> if (cmd < 0)
> return cmd;
>
> mutex_lock(&tlmi_mutex);
>
> switch (cmd) {
> case save_single:
> tlmi_priv.save_mode = TLMI_SAVE_SINGLE;
> goto out;
> case save_bulk:
> tlmi_priv.save_mode = TLMI_SAVE_BULK;
> goto out;
> case save_save:
> break; /* Continue with saving settings */
> }
>
> /* The user is triggering a save - if supported */
> if (!tlmi_priv.can_set_bios_settings ||
> tlmi_priv.save_mode == TLMI_SAVE_SINGLE)
> return -EOPNOTSUPP;
>
> ...
>
> This lets sysfs_match_string() do the string parsing work
> for us, getting rid of having to do this ourselves.
>
Agreed - this is much better. Thanks for the suggestion and I'll make the change.
> Notice I have also moved the mutex_lock() up, so that
> it is also done for updating the save_mode since we
> don't want that the change halfway through a possibly
> racing current_value_store() call.
>
Ack
Mark
