<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">&lt;<a href="mailto:jhrozek@redhat.com" target="_blank">jhrozek@redhat.com</a>&gt;</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>
&gt; Dear SSSD Developers,<br>
&gt; please review attached patch for issue 1756<br>
&gt;<br>
&gt; Description:<br>
&gt;<br>
&gt; [PATCH] Patch for bug #1756, Append new line to string from poptStrerror()<br>
&gt;<br>
&gt; This patch changes function usage() in tools_util.c which is used by<br>
&gt; BAD_POPT_PARAMS macro.<br>
&gt; Since BAD_POPT_PARAMS macro is called by multiple functions in multiple<br>
&gt; files with both custom messages termited with \n as well as popt messages<br>
&gt; which are not terminated by default, this patch offers a correction by<br>
&gt; checking the string just after printing out to console and printing out<br>
&gt; additional \n if new line wasn&#39;t present.<br>
&gt;<br>
&gt; Best wishes<br>
&gt; 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&#39;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 &quot;&quot; or &quot;\n&quot; depending on the condition. But<br>
because this is just an error handler, it&#39;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&#39;s somewhat similar to<br>
   kernel&#39;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&#39;ll add it<br>
   there.<br>
<br>
If you fix the above small problems, I&#39;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>