[SSSD] [PATCH] First Boot Seed Tool

Nick Guay nguay at redhat.com
Fri Jul 27 19:30:30 UTC 2012


On 25/07/12 11:01, Pavel Březina wrote:
> Nack.
>
> Things to consider:
>
> - support reading password directly from command line
>   - -p 123 | -password 123
>   - -f file | --password-file file
> - support reading everything from file that contains users in passwd
>   format
>
> Please include the manual page in src/man/Makefile.am and src/man
> /po/po4a.cfg so it can be built and translated.
>
> A forward backslash is used to close a tag (ie </para> instead of 
> <\para>.
>
> sss_seed.c:
>
> Mark the functions as static.
>
> Don't use tools_ctx and octx and create a custom structure (say 
> seed_ctx) that will fit exactly to your needs.
>
> Our convention is to use tmp_ctx for temporary contexts.
>
> You don't use enum seed_input_type anywhere.
>
> 180: Missing NULL check.
> 191: Typo "Passowrds"
> 197: Create a local variable "char *password" and function parameter 
> "char **_password". Work with the "password" and then steal it at the 
> end:
> *_password = talloc_steal(mem_ctx, password).
>
> seed_interactive_input(): Debug messages would be nice here when 
> something fails.
>
> 296: This check should be done at the beginning of the function.
> 300: Use lower case in "domain".
> 319, 320: The same thing as with the password variable.
>
> main(): Load every option into separate variable (pc_*) and 
> talloc_strdup() that into the structure (see sss_usermod) so you don't 
> mix malloc and talloc pointers.
>
> 490: Use CHECK_ROOT macro here.
>
> 500: You should use fully qualified name (user at domain) to be sure that 
> you get the correct user in multi domain environment. If UID is 
> specified you should test if it matches passwd->pw_uid.
>
> Use errno to get proper error code from getpwnam(). The good way would 
> be:
>
> errno = 0;
> pc_passwd = getpwnam(...);
> if (pc_passwd == NULL) {
>     ret = errno;
>     DEBUG(..., ("... [%d]: %s\n", ret, strerror(ret)));
>     goto done;
> }
>
> You should copy (talloc_strdup()) passwd values you need instead of 
> simple assignment because it points to a static location which may be 
> overwritten by other calls.
>
> Also name pc_passwd doesn't make sense as it has nothing in common 
> with popt context.
>
> 518: You're missing space after comma in the comment.
>
> Put everything after line 579 into a separate function.
>
> Thanks,
> Pavel.
>

Thanks, Pavel. I covered most of what you noted. I forgot to include one 
commit with the last patch I sent, which left the interactive input 
broken in the last patch. That should be fixed along with a shorter, 
more readable main function.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-First-boot-sss_seed-tool.patch
Type: text/x-patch
Size: 37854 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120727/f2d5fac5/attachment.bin>


More information about the sssd-devel mailing list