[SSSD] Re: [PATCH] Warn if ad_server contains IP address

[Justin Stephenson]

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
> > > [e-mail]
> > > https://lists.fedorahosted.org/admin/lists/[e-mail]
>
> The code itself is good, but I have two more comments:
> 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
> 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
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]