On Wed, Aug 10, 2016 at 12:09:10PM -0400, Justin Stephenson wrote:
On 08/09/2016 05:41 AM, Jakub Hrozek wrote:
> On Fri, Aug 05, 2016 at 12:09:27PM -0400, Justin Stephenson wrote:
> > Hi Lukas,
> >
> > I sent a response on July 6th but perhaps there was an issue with the
> > mailing list or some reason it did not go through.
> Yes, we had issues with the mailing list back then (it was a Fedora
> mailman bug that was fixed in the meantime)
>
> > Updated patch attached.
> >
> > I moved the resolv_is_address() function declaration into the
> > async_resolv.h file(so that it could be included in the
> > cmocka/test_resolv_fake.c test but I am not sure if this is the
> > correct approach). I also made the assumption of including my tests
> > in the already existing test_resolv_fake.c file instead of a
> > different file.
> >
> > Also, I wasn't sure whether to use SSSDBG_MINOR_FAILURE or
> > SSSDBG_CONF_SETTINGS debug log level.
> I would say SSSDBG_IMPORTANT_INFO, we want to be loud here.
>
> > I am sure there are some corrections to make so I appreciate any
> > feedback.
> I haven't tested the patch yet, just read the diff, but the code looks
> OK to me. I would just suggest to split the patch into two, one that
> makes the resolv function public and adds the test and the other that
> uses the function in the providers.
I split the patch into 2 separate patches as requested, see attached.
Testing with ad_server = <ad-ip-address> was successful with the debug
message being printed in the domain log.
Kind regards,
Justin Stephenson
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
The code itself is good, but I have two more comments:
1) the patches are reversed, the first should introduce the new public
functions and only then can the second patch use them. You can use "git
rebase -i origin/master" and just swap the two commit lines
2) I don't like the commit messages :) I would drop the "Part X of of
fix" altogether and use the next sentence as the first line of the
commit message. So for the first patch this would be:
Subject: [PATCH] Make resolv_is_address() function public and create some basic tests
Resolves:
https://fedorahosted.org/sssd/ticket/2789