<div dir="ltr"><div><div>Dear Mr. Hrozek,<br></div>I really appreciate all your advice. I tried to correct mentioned mistakes, changed the code to call the fprintf() only once and reposted the patch anew with now hopefully correct subject format and with ticket URL included.<br>
<br></div>Best Regards,<br>Milan Cejnar<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Feb 18, 2013 at 12:44 PM, Jakub Hrozek <span dir="ltr"><<a href="mailto:jhrozek@redhat.com" target="_blank">jhrozek@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Feb 18, 2013 at 10:40:23AM +0100, Milan Cejnar wrote:<br>
> Dear SSSD Developers,<br>
> please review attached patch for issue 1756<br>
><br>
> Description:<br>
><br>
> [PATCH] Patch for bug #1756, Append new line to string from poptStrerror()<br>
><br>
> This patch changes function usage() in tools_util.c which is used by<br>
> BAD_POPT_PARAMS macro.<br>
> Since BAD_POPT_PARAMS macro is called by multiple functions in multiple<br>
> files with both custom messages termited with \n as well as popt messages<br>
> which are not terminated by default, this patch offers a correction by<br>
> checking the string just after printing out to console and printing out<br>
> additional \n if new line wasn't present.<br>
><br>
> Best wishes<br>
> Milan Cejnar<br>
<br>
</div></div>Hi Milan,<br>
<br>
Thank you for the contribution!<br>
<br>
In general I like the patch and it's good that it covers both the cases<br>
where the caller already appends \n and cases where you want the usage()<br>
to append the \n itself.<br>
<br>
Normally, it might be more efficient to call fprintf just once with a<br>
string pointer set to either "" or "\n" depending on the condition. But<br>
because this is just an error handler, it's not a blocker issue.<br>
<br>
I have a couple of very minor comments:<br>
* Our coding style recommends to use {} brackets even for one-line<br>
conditions<br>
* In general when sending patches we prefer to add a [PATCH] tag into<br>
the subject tag of the e-mail, too. This is done to make it easier<br>
for outside developers to follow the discussions but not the patch<br>
reviews.<br>
* We also usually put the short description as the first line, an empty<br>
line, the full URL of the ticket fixed, an empty line and then a<br>
short description of the problem fixed. It's somewhat similar to<br>
kernel's process (see<br>
<a href="http://www.kernel.org/doc/Documentation/SubmittingPatches" target="_blank">http://www.kernel.org/doc/Documentation/SubmittingPatches</a> bullet point<br>
15) for example) I realize this info is missing from<br>
<a href="https://fedorahosted.org/sssd/wiki/DevelTutorials" target="_blank">https://fedorahosted.org/sssd/wiki/DevelTutorials</a> and I'll add it<br>
there.<br>
<br>
If you fix the above small problems, I'll apply the patch.<br>
_______________________________________________<br>
sssd-devel mailing list<br>
<a href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a><br>
<a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
</blockquote></div><br></div>