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

Pavel Březina pbrezina at redhat.com
Thu Oct 22 12:26:08 UTC 2015


On 10/22/2015 02:10 PM, Pavel Reichl wrote:
>
>
> 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.

What I don't like about this is that it does not encapsulate any 
difficult logic. You just hide once function call into another function.

This type of change in my opinion leads to other changes:
- why don't you add list_user_overrides into the function?
- then you can put there also do cycle
- then you can put there colondb initialization
- then talloc context
- and you have nothing left :-)

But we can do it, if you think it will improve readability.

>
>>>
>>> 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
>>
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>



More information about the sssd-devel mailing list