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@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@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
