Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
>
> +static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
> + struct sysdb_attrs *attrs,
> + const char *ext_key,
> + char ***_missing)
^
Coding style. Remove the space between function name and "(".
Do not forget to align the parameters after that.
> +{
> + bool is_present = false;
> + size_t size = 0;
> + size_t ret;
> +
> + for (int i = 0; i < attrs->num; i++) {
> + if (strcmp(ext_key, attrs->a[i].name) == 0) {
> + is_present = true;
> + }
> + size++;
> + }
> +
> + if (is_present == false) {
> + ret = add_string_to_list(attrs, ext_key, _missing);
> + if (ret != EOK) {
> + goto fail;
> + }
> + }
> +
> + ret = EOK;
> +
> +fail:
Please change the label name to "done". According to
our new coding style, the code that follows label "fail"
is only executed when failure occurs. I know we do not
follow this everywhere, but I would like to be consistent
in new code.
> + return ret;
> +}
> +
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do
not want to postpone this patch before release, so you can
do it later as part of this ticket:
https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or
assign the above ticket to yourself and do it
when there is more time.
There is not "either or" for this case :-)
Our experience(e.g. #3045 and many others) in sssd is that
if test is not written together with fix then it will be very very difficult
to write it later.
e.g. There isn't time; there are tasks with higher priority ...
That's the reason why test for this particular bug must be pushed together with
the fix.
We should consider ticket #3119 as an enhancement of our testing of netgroups
and not as a replacement of testing of this bug.
I hope we are all on the same page.
LS