[SSSD] [PATCH] sss_override: add -find and -show

Pavel Reichl preichl at redhat.com
Wed Oct 21 13:44:20 UTC 2015


Hello, thanks for patches. Please see comments inline.

> +static errno_t user_export(const char *filename,
> +                           struct sss_domain_info *dom,
> +                           bool iterate)
> +{
> +    TALLOC_CTX *tmp_ctx;
> +    struct sss_colondb *db;
> +    struct override_user *objs;
> +    errno_t ret;
> +    int i;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> +        return ENOMEM;
> +    }
> +
> +
One empty line too many
> +    db = sss_colondb_open(tmp_ctx, SSS_COLONDB_WRITE, filename);
> +    if (db == NULL) {
> +        fprintf(stderr, _("Unable to open %s.\n"),
> +                filename == NULL ? "stdout" : filename);
> +        ret = EIO;
> +        goto done;
> +    }
> +
> +    do {
> +        objs = list_user_overrides(tmp_ctx, dom);

Would you consider moving definition of objs closer to it's usage?

> +        if (objs == NULL) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get override objects\n");
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +
> +        for (i = 0; objs[i].orig_name != NULL; i++) {
You can move definition of i closer to its usage.
> +            /**
> +             * Format: orig_name:name:uid:gid:gecos:home:shell
> +             */
> +            struct sss_colondb_write_field table[] = {
> +                {SSS_COLONDB_STRING, {.str = objs[i].orig_name}},
> +                {SSS_COLONDB_STRING, {.str = objs[i].name}},
> +                {SSS_COLONDB_UINT32, {.uint32 = objs[i].uid}},
> +                {SSS_COLONDB_UINT32, {.uint32 = objs[i].gid}},
> +                {SSS_COLONDB_STRING, {.str = objs[i].gecos}},
> +                {SSS_COLONDB_STRING, {.str = objs[i].home}},
> +                {SSS_COLONDB_STRING, {.str = objs[i].shell}},
> +                {SSS_COLONDB_SENTINEL, {0}}
> +            };
> +
> +            ret = sss_colondb_writeline(db, table);
> +            if (ret != EOK) {
> +                DEBUG(SSSDBG_CRIT_FAILURE, "Unable to write line to db\n");
> +                goto done;
> +            }
> +        }
> +
> +        if (!iterate) {
> +            break;
> +        }
I think that this condition is needless as it's already part of condition in 'while' check.
> +
> +        /* All overrides are under the same subtree, so we don't want to
> +         * descent into subdomains. */
> +        dom = get_next_domain(dom, false);

Changing value of input param. is not so great...but we already do so on many many places...

> +    } while (dom != NULL && iterate);
> +
> +    ret = EOK;
> +
> +done:
> +    talloc_free(tmp_ctx);
> +
> +    return ret;
> +}
I think you could move the body of do-while cycle to a separate static function and thus improve code readability (function user_export() would be shorter, name of new function would help to understand the code flow and we would avoid having cycle in 
cycle and deeper indentation).
You could also consider if you see any way how to avoid code duplication that is between user_export() and group_export() but take this just as a proposal (from which you would benefit when you will do the same changes I requested for group_export() 
:-) ).


> +static int override_user_find(struct sss_cmdline *cmdline,
> +                             struct sss_tool_ctx *tool_ctx,
> +                             void *pvt)

bad indentation

> +{
> +    struct sss_domain_info *dom;
> +    bool iterate;
> +    errno_t ret;
> +
> +    ret = parse_cmdline_find(cmdline, tool_ctx, &dom);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");

You don't seem to use '.' at other debug messages except for fprintf() statement in user_export() where I thought it was on purpose.
> +        return EXIT_FAILURE;
> +    }
> +
> +    if (dom == NULL) {
> +        dom = tool_ctx->domains;
> +        iterate = true;
> +    } else {
> +        iterate = false;
> +    }
> +
> +    ret = user_export(NULL, dom, iterate);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to export users\n");
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
> +}
> +


> +static int override_user_show(struct sss_cmdline *cmdline,
> +                              struct sss_tool_ctx *tool_ctx,
> +                              void *pvt)
> +{
> +    TALLOC_CTX *tmp_ctx;
> +    struct override_user input = {NULL};
> +    const char *dn;
> +    char *anchor;
> +    const char *filter;
> +    int ret;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");

trailing dot?

> +        return EXIT_FAILURE;
> +    }
> +
> +    ret = parse_cmdline_user_show(cmdline, tool_ctx, &input);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
trailing dot?
> +        goto done;
> +    }
> +
> +    ret = get_user_domain_msg(tool_ctx, &input);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get object domain\n");
> +        goto done;
> +    }
> +
> +    ret = get_object_dn(tmp_ctx, input.domain, SYSDB_MEMBER_USER,
> +                        input.orig_name, NULL, &dn);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get object dn\n");
> +        goto done;
> +    }
> +
> +    anchor = build_anchor(tmp_ctx, dn);
> +    if (anchor == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    ret = sss_filter_sanitize(tmp_ctx, anchor, &anchor);
> +    if (ret != EOK) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    filter = talloc_asprintf(tmp_ctx, "(%s=%s)",
> +                             SYSDB_OVERRIDE_ANCHOR_UUID, anchor);
> +    if (filter == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf() failed\n");
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    ret = user_export(NULL, input.domain, false, filter);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to export users\n");
> +        return EXIT_FAILURE;

Leak?

> +    }
> +
> +    ret = EOK;
> +
> +done:
> +    talloc_free(tmp_ctx);
> +
> +    if (ret != EOK) {
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
> +}
> +

IMO you can make function shorter and more readable by extracting function to obtain filter and function for to obtaining input. What do you think? Might be you could reuse them in override_group_show()?






More information about the sssd-devel mailing list