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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel