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