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

Pavel Reichl preichl at redhat.com
Thu Oct 22 12:10:19 UTC 2015



On 10/22/2015 01:18 PM, Pavel Březina wrote:
> On 10/22/2015 01:17 PM, Pavel Březina wrote:
>> 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 :-)

I attached ad-hoc patch illustrating the idea I had in mind.

>>
>> 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.
No, I don't insist. I just think it would be good to be consistent at least on function/patch level, but I don't really mind.

>>
>>>> +
>>>> +    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.
You are right I missed different types at some places. Probably not worth the effort, thanks for checking.

>
> Now with patches.
>
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-tmp.patch
Type: text/x-patch
Size: 3175 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151022/3bd554f8/attachment.bin>


More information about the sssd-devel mailing list