On Thu, May 27, 2021 at 5:36 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > On 5/27/21 4:16 PM, Andy Shevchenko wrote: > > On Wed, May 26, 2021 at 11:15 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote: ... > >> + char *set_str = NULL, *new_setting = NULL; > >> + char *auth_str = NULL; > > > > The rule of thumb is to avoid forward assignments on stack allocated > > variables. It may decrease readability, hide real issues, and simply > > be unneeded churn, like here. Please revisit all of them in entire > > series. > > I asked for all this to be set to NULL in my review of v2, > since there are various "goto out"s in the function and out: > calls kfree() on all 3, in v2 there was a path which would > end up calling kfree() on an uninitialized char *. IMHO just > initializing all of them up front is best here because that > guarantees that they are either still NULL, or point to > memory returned by kmalloc. I see your point, fine with me! ... > >> + /* Remove the value part */ > >> + strreplace(item, ',', '\0'); > > > > This is kinda non-standard pattern. > > > > I would see rather something like > > > > char *p; > > > > p = strchrnul(item, ','); > > *p = '\0'; > > > > Yes, it's longer, but better to understand what's going on here. > > Erm, you actually suggested using strreplace() here in your previous review ... There is strreplace() in use somewhere still which is what I suggested. It might be that I missed the fact that in some places the change happens to the ''\0' rather than another non-NUL character. So, strreplace() is basically for changing '$a' to '$b' where neither of them is NUL. Otherwise we have - strsep() - strchrnul() - strtrim() depending on the case. Sorry if I was not completely clear about something. ... > >> + if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE)) > >> + pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1; > > > > Not sure if WARN_ON() is really what has to be called here. But I > > haven't checked the context deeply. > > There are 2 max_lengths, one hardcoded in the driver so we don't > need to dynamically manage memory for the password storage and one > which is actually queried from the BIOS, the BIOS max-length should > always be less then the hardcoded one in the driver (and currently > this is true for all known models). > > I suggested adding the WARN_ON so that if future BIOS-es ever bump > max_length we will get a bug-report about this and then we can > bump the driver's TLMI_PWD_BUFSIZE value. I see your point, thanks for elaboration. -- With Best Regards, Andy Shevchenko
