[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