Thanks Andy On 2021-05-25 12:18 p.m., Andy Shevchenko wrote: > On Tue, May 25, 2021 at 6:14 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote: >> On 2021-05-22 7:04 a.m., Andy Shevchenko wrote: >>> On Sun, May 9, 2021 at 5:02 AM Mark Pearson <markpearson@xxxxxxxxxx> wrote: > > ... > >>>> + *string = kstrdup(obj->string.pointer, GFP_KERNEL); >>>> + kfree(obj); >>>> + return *string ? 0 : -ENOMEM; >>> >>> This breaks the principle "don't touch the output in error case". >> >> But I'm not changing *string in an error case here - I'm not >> understanding the issue here. >> Happy to rewrite it to make it clearer though if that would help. > > *string may be not NULL when you do assign it. > You need to assign it iff you are about to return 0. Ah - got it. I'll fix. > > ... > >>>> + length = strlen(buf); >>>> + if (buf[length-1] == '\n') >>>> + length--; >>> >>> This will prevent you from using \n in the password. Why? >> The BIOS doesn't like it - so we strip it out :) > > I haven't checked, but if there is no description of this in the > documentation/commit message, should be added. Ack. > > ... > >>>> + memcpy(setting->password, buf, length); >>> >>>> + setting->password[length] = '\0'; >>> >>> Why is the password a *string*? From where that assumption comes from? >> Sorry, I'm not understanding the question here. It's what our BIOS is >> expecting. I'm missing something here > > So, BIOS restrictions should be documented if not yet. Ack > > ... > >>>> + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */ >>>> + len = strlen(setting->password) + strlen(encoding_options[setting->encoding]) >>>> + + strlen(setting->kbdlang) + 3 /* type */ >>>> + + strlen(new_pwd) + 6 /* punctuation and terminator*/; >>>> + auth_str = kzalloc(len, GFP_KERNEL); >>>> + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", >>>> + setting->pwd_type, setting->password, new_pwd, >>>> + encoding_options[setting->encoding], setting->kbdlang); >>> >>> NIH of kasprintf() >> Not sure what NIH is - > > https://en.wikipedia.org/wiki/Not_invented_here Neat :) > >> but I'm assuming I should be using kasprintf >> instead of snprinf :) >> I wasn't aware of it - thank you. > > strlen+kmalloc+sprintf == kasprintf > > ... > >>> The terminator line doesn't need a comma. >> Argh. I always get this wrong as to when it is required and when it isn't. >> I'll fix > > If it is supposed to be the last entry (i.o.w. terminator) --> no comma. > Thanks
