On (03/08/16 13:29), Jakub Hrozek wrote:
>On Wed, Aug 03, 2016 at 12:52:31PM +0200, Petr Cech wrote:
>> On 08/03/2016 12:46 PM, Lukas Slebodnik wrote:
>> > On (03/08/16 12:34), Michal Židek wrote:
>> > > 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
>>
>> Hi all,
>>
>> Michal, thanks for review. Lukas, yes, we are on the same page, I hope.
>> Tests are important. I would like to write tests during rest of week.
>
>I wrote that in the ticket Michal filed, but I don't see support for
>netgroups in nss_wrapper. But I don't think it should be too difficult
>to add..
Faster would be to do the same as with initgroups (sssd_id.py)
If is is too difficult we can do it as a part of #3119
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
