[SSSD] [PATCH] First Boot Seed Tool

Nick Guay nguay at redhat.com
Tue Jul 31 16:08:40 UTC 2012


On 30/07/12 12:02, Pavel Březina wrote:
> 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 at 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-tmpctx-2
> [2] 
> http://talloc.samba.org/talloc/doc/html/libtalloc__context.html#context-hierarchy
> [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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-First-boot-sss_seed-tool.patch
Type: text/x-patch
Size: 39042 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/1e566bf1/attachment.bin>


More information about the sssd-devel mailing list