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@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.
Avoid stealing talloc context on NULL. You should basically add mem_ctx
parameter to the function and use it instead of NULL. You can find more
information about usage of temporary contexts at [1]
Create and obey good talloc context hierarchy. How to do that can be
found at [3][4].
sss_seed.c:304 Trailing whitespace.
struct seed_ctx:
- remove member 'transaction_done', it is used only in one function
and does not describe state of the tool, so it should be a local
variable. The same applies to snctx.
- remove member 'error', it is not used at all
- remove member 'domain', you initialize it but don't use it
main():
- don't mix errno codes (like ENOMEM) with stdlib EXIT_FAILURE, use
errno codes and convert them to EXIT_FAILURE/SUCCESS after 'done' label
- when -i option is specified, you want to print an error if user is
cached or allow replacing sysdb entry with provided values
- line 869: this check should be done even when user_cached == true
- provide uctx as a memory context in seed_password_input() and
seed_check_groups() calls
- this is not necessary if you will create a good context hierarchy
(you need just talloc_free(sctx)
if (sctx != NULL) {
talloc_zfree(sctx->uctx);
}
talloc_free(sctx);
seed_init():
- don't mix errno codes (like ENOMEM) with stdlib EXIT_FAILURE, return
only errno codes and transfer them in the caller if necessary
- allow specifying username either via option or as a left over
argument, not both
seed_dom_user_info():
- getpwnam() as I wrote before, you should use a fully qualified name
here, you may get wrong user info otherwise (it may be a local user with
the same name, or user from different domain)
- you need to talloc_zfree() uctx members before setting a new value,
you are leaking memory otherwise. But you don't actually need to fill
uctx here, because once the user is cached, you are changing only password.
- this is very wrong usage of talloc_steal() because you are
destroying talloc context hierarchy, more information at [2][3]. Simply
remove it and it will be ok.
seed_dom_user_info() should also contain sysdb_getpwnam(). Calling
getpwnam() is used to force sssd to load user information into the
cache. So what you should do is:
static int seed_cached_user_info(TALLOC_CTX *mem_ctx,
const char *username,
const char *domainname)
passwd = getpwnam(user@domain);
sysdb_getpwnam(...);
if (cached) {
return EOK;
} else {
return ENOENT;
}
In main():
ret = seed_cached_user_info(sctx, ..., &new_uctx);
if (ret == EOK) {
DEBUG(user is cached, only the password will be set);
} else if (ret == ENOENT) {
using provided values
} else {
error
}
seed_init_db():
- use const char *domain_name
- use **_confdb(...) in the parameter list and *confdb(...) as a local
variable, steal it to mem_ctx only at the end when everything goes fine:
*_confdb = talloc_steal(mem_ctx, confdb);
*_sysdb = talloc_steal(mem_ctx, sysdb);
*_domain = talloc_steal(mem_ctx, domain);
done:
talloc_free(tmp_ctx);
return ret;
seed_interactive_input():
- provide tmp_ctx as a memory context and the steal the values to uctx
seed_prompt(): no need for tmp_ctx here, use:
prompt = talloc_asprintf(NULL, _("Enter %s:"), req);
...
talloc_free(prompt);
seed_str_input(): no need for tmp_ctx here, use:
*_input = talloc_strdup(mem_ctx, buf);
line 202: can you put the PASS_MAX macro at the beginning please?
seed_password_input(): split this function into two (one for reading
from file and one for reading from stdin)
seed_check_groups():
- you are leaking memory on errors, use tmp_ctx pattern here as well
- check context hierarchy - addgroups[i] should be allocated on
addgroups which should be allocated on uctx
- **_var, *var, talloc_steal() pattern would be nice here too
More info about stealing at [4]
[1]
http://talloc.samba.org/talloc/doc/html/libtalloc__bestpractices.html#bp-...
[2]
http://talloc.samba.org/talloc/doc/html/libtalloc__context.html#context-h...
[3]
http://talloc.samba.org/talloc/doc/html/libtalloc__bestpractices.html
[4]
http://talloc.samba.org/talloc/doc/html/libtalloc__stealing.html