Thanks Jakub for your help, updated patch set attached.
Kind regards,
Justin Stephenson
On 08/12/2016 06:05 AM, Jakub Hrozek wrote:
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
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org