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
Fixed the talloc problems noted, and also fixed the mismatched error
codes. The username option is no longer a standalone option. getpwnam()
and initgroups() now use the fully qualified name. If the user is
cached, only the password is updated for the cached entry. An error is
now produced and interactive input is skipped if the -i option is
provided for a cached user.