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

Pavel Březina pbrezina at redhat.com
Thu Oct 22 11:17:45 UTC 2015


On 10/21/2015 03:44 PM, Pavel Reichl wrote:
> Hello, thanks for patches. Please see comments inline.
>
>> +    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.

No to both considerations. In general I don't oppose to this coding 
style change but I would like to keep the style consistent within modules.

>> +        if (!iterate) {
>> +            break;
>> +        }
> I think that this condition is needless as it's already part of
> condition in 'while' check.

Removed.

>
>> +    } 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() :-) ).

No, not really. There is nothing to cut away, if you look at it closely. 
Once you remove the loop, you can also remove colondb initialization 
since it belongs to the loop and there is nothing left in the function :-)

The code duplication can't be removed. The logic is the same, but the 
code is completely different. It uses different data type (override_user 
vs override_group), different list function, different colondb table so 
it is not a code duplication at all and you can't merge it without 
pointer magic AFAIK. Simple way would be macro which I'd like to do, but 
you guys seem to be very resilient to code blocks macros :-)

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

Fixed.

>
>> +{
>> +    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.

Well, there are plenty of places were I use dot at the end in the whole 
module :-) Sometimes I add it sometimes I don't. I can prepare a patch 
that makes it consistent in the whole module if you want me to.

>> +
>> +    ret = user_export(NULL, input.domain, false, filter);
>> +    if (ret != EOK) {
>> +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to export users\n");
>> +        return EXIT_FAILURE;
>
> Leak?
>

Yes! Fixed.

>> +    }
>> +
>> +    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()?

The problem is that even though variable names are the same, data types 
and functions are different so you can't simply reuse it without pointer 
magic.




More information about the sssd-devel mailing list