On Fri, May 28, 2021 at 8:36 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote: > On 2021-05-27 10:16 a.m., Andy Shevchenko wrote: > > On Wed, May 26, 2021 at 11:15 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote: ... > >> +#include <linux/acpi.h> > > > > linux/errno.h ? > > > >> +#include <linux/fs.h> > > > > linux/string.h ? > > linux/types.h ? > > > >> +#include <linux/wmi.h> > >> +#include "firmware_attributes_class.h" > >> +#include "think-lmi.h" > > I hadn't included those as they call get pulled in by linux/acpi.h - so > aren't needed here. I take it best practice is to add them. I'd > deliberately stripped down the list to the bare minimu previously but > happy to reverse that if it's preferred. Basic headers like that are kinda what almost any file uses, but acpi.h is not the one which guarantees their inclusion (you see, it's a layering violation: acpi is not low level generic library). ... > >> +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled); > > > > Hmm... We have already define_one_global_ro(). The problems with that > > are the name and location. But perhaps you can copy that macro here > > with the same name and at least we can see the common grounds to clean > > up in the future. Another possibility is to comment that it can be > > replaced with the above mentioned macro. Ideally would be to refactor > > right now, but it's not anyhow crucial or required for this series, so > > may be postponed. > OK - I'll have a look. I'm not 100% sure I understand the issues, but > hopefully it will become clearer once I play with it a bit Including cpufreq.h in your code will seem weird. That's why I marked it as an issue. ... > >> + pwdlen = strlen(buf); > >> + if (buf[pwdlen-1] == '\n') > >> + pwdlen--; > > > > But the question is what will happen with the string like > > 'pa55\nw0rd\n' (note all \n:s)? > > See also below. > I know there is discussion on this in future mails, so I'll leave this > one for those OK. ... > >> + /* 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. > Fair enough. I'll change. I quite liked the fact I had a one-liner do > the same thing - oh well :) Me too, but the problem with strreplace(..., '\0') is that it sounds like a half-baked strsep() solution. First of all, it will go and replace all \n to \0 while you stop anyway on the first one in the following code. -- With Best Regards, Andy Shevchenko
