[SSSD] [PATCH] First Boot Seed Tool

Jakub Hrozek jhrozek at redhat.com
Mon Jul 30 16:35:29 UTC 2012


On Mon, Jul 30, 2012 at 06:02:45PM +0200, Pavel Březina wrote:
> On 07/27/2012 09:30 PM, Nick Guay wrote:
> >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.
> 
> Nack.
> 
> The code is way better now. Especially the main function. Hovewer,
> there occurred several more problems, mostly about talloc contexts.
> I highly suggest you reading talloc tutorial
> (http://talloc.samba.org/talloc/doc/html/libtalloc__tutorial.html),
> especially the links that are at the bottom of this mail.

Hi,

quite a few of the issues described in the review is code style. While
it is important to get it right, we don't absolutely need to get them
all fixed by beta.

It's necessary to fix the username being passed as both positional
argument and with an option, because the popt strings are translatable.

We could file a cleanup ticket for the rest and deal with it post-beta
if it would take too much time to get them fixed all.



More information about the sssd-devel mailing list