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. ... > >> + 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. ... > >> + 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. ... > >> + /* 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 > 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. -- With Best Regards, Andy Shevchenko
