[SSSD] Patch for issue 1756

Jakub Hrozek jhrozek at redhat.com
Mon Feb 18 11:44:10 UTC 2013


On Mon, Feb 18, 2013 at 10:40:23AM +0100, Milan Cejnar wrote:
> Dear SSSD Developers,
> please review attached patch for issue 1756
> 
> Description:
> 
> [PATCH] Patch for bug #1756, Append new line to string from poptStrerror()
> 
> This patch changes function usage() in tools_util.c which is used by
> BAD_POPT_PARAMS macro.
> Since BAD_POPT_PARAMS macro is called by multiple functions in multiple
> files with both custom messages termited with \n as well as popt messages
> which are not terminated by default, this patch offers a correction by
> checking the string just after printing out to console and printing out
> additional \n if new line wasn't present.
> 
> Best wishes
> Milan Cejnar

Hi Milan,

Thank you for the contribution!

In general I like the patch and it's good that it covers both the cases
where the caller already appends \n and cases where you want the usage()
to append the \n itself.

Normally, it might be more efficient to call fprintf just once with a
string pointer set to either "" or "\n" depending on the condition. But
because this is just an error handler, it's not a blocker issue.

I have a couple of very minor comments:
 * Our coding style recommends to use {} brackets even for one-line
   conditions
 * In general when sending patches we prefer to add a [PATCH] tag into
   the subject tag of the e-mail, too. This is done to make it easier
   for outside developers to follow the discussions but not the patch
   reviews.
 * We also usually put the short description as the first line, an empty
   line, the full URL of the ticket fixed, an empty line and then a
   short description of the problem fixed. It's somewhat similar to
   kernel's process (see
   http://www.kernel.org/doc/Documentation/SubmittingPatches bullet point
   15) for example) I realize this info is missing from
   https://fedorahosted.org/sssd/wiki/DevelTutorials and I'll add it
   there.

If you fix the above small problems, I'll apply the patch.



More information about the sssd-devel mailing list