<div dir="ltr"><div>Thanks for review, Please find updated patch and comments inline bellow.</div><div><br></div><div>Regards</div><div>Umar Hayat</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Mar 14, 2020 at 12:07 PM Tatsuo Ishii <<a href="mailto:ishii@sraoss.co.jp">ishii@sraoss.co.jp</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Umar,<br>
<br>
> Any feedback ?<br>
<br>
I have looked into this.<br>
<br>
(1) I think error message style is a little bit different from<br>
the standard of ours (or PostgreSQL, we follow PostgreSQL style).<br>
<br>
+ fprintf(stderr, "input_file \"%s\" cannot be opened\n\n", input_file);<br>
According to our style this will be something like this. Also it's recommended to add %m to show the cause of the errro.<br>
fprintf(stderr, "failed to open input_file \"%s\" (%m)\n\n", input_file);<br>
<br></blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
(2) It's not recommended to start an error message with upper case letter.<br>
+ fprintf(stdout, "Input exceeds maximum username length!\n\n");<br>
Also I think it's better to inform the max username length. I propose something like:<br>
+ fprintf(stdout, "input exceeds maximum username length %d\n\n", MAX_USER_NAME_LEN);<br>
<br>
(3) Following message style seems to be inconsistent the message above.<br>
<br>
+ fprintf(stdout, "Input exceeds maximum password length given:%d max allowed:%d!\n", (int)strlen(pch), MAX_PGPASS_LEN);<br>
What about this?<br>
+ fprintf(stdout, "input exceeds maximum password length %d\n", MAX_PGPASS_LEN);<br>
<br></blockquote><div>For (2), (3), I followed/copied existing pattern of error string already there in utility. In latest patch I converted strings that I added to lower case and updated string as you suggested.</div><div>( I didn't updated strings which I just indented/or moved, there are number of other location where above format is not followed, let me know if I send a separate patch to fix them in multiple utilities/files )</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
(4) if username+password exceeds the buffer for fgets(), pg_enc<br>
errored out but read rest of the line which fgets() did not read and<br>
will be confused.<br>
<br>
LINE#01: USER: username1<br>
LINE#02: USER: username2<br>
LINE#03: Input exceeds maximum username length!<br>
<br>
LINE#04: Invalid username:password pair<br>
LINE#05: USER: username4<br>
<br>
Actually there's no line#05. See attached test input.<br>
<br></blockquote><div>Fixed. I also changed verbosity to error only. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
(5) I am not sure if we need to check the input by using stat():<br>
<br>
+ if (stat(input_file, &stat_buf) != 0)<br>
+ {<br>
+ fprintf(stderr, "input_file \"%s\" cannot be opened. Check file permissions\n\n", input_file);<br>
+ exit(EXIT_FAILURE);<br>
+ }<br>
<br>
- return EXIT_SUCCESS;<br>
+ if (!S_ISREG(stat_buf.st_mode))<br>
+ {<br>
+ fprintf(stderr, "input_file \"%s\" is not a plain file\n\n", input_file);<br>
+ exit(EXIT_FAILURE);<br>
+ }<br>
<br>
I think it's overkill. Usually we (and PostgreSQL) just try to open a<br>
file and complain if it fails.<br>
<br></blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
(6) <br>
+ buf[len - 1] = 0;<br>
It's better to use '\0' rather than 0 here.<br>
<br></blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Best regards,<br>
--<br>
Tatsuo Ishii<br>
SRA OSS, Inc. Japan<br>
English: <a href="http://www.sraoss.co.jp/index_en.php" rel="noreferrer" target="_blank">http://www.sraoss.co.jp/index_en.php</a><br>
Japanese:<a href="http://www.sraoss.co.jp" rel="noreferrer" target="_blank">http://www.sraoss.co.jp</a><br>
<br>
> On Tue, Mar 10, 2020 at 5:07 PM Umar Hayat <<a href="mailto:m.umarkiani@gmail.com" target="_blank">m.umarkiani@gmail.com</a>> wrote:<br>
> <br>
>> Hi Hackers,<br>
>> Attached patch address mentisbt-422<br>
>> <<a href="https://www.pgpool.net/mantisbt/view.php?id=422" rel="noreferrer" target="_blank">https://www.pgpool.net/mantisbt/view.php?id=422</a>> for pg_enc utility.<br>
>> User can provide user name , password pair in a file to encrypt and add<br>
>> them to pool_passwd file.<br>
>> A new --input-file option is added in pg_enc. Using this pg_enc will parse<br>
>> the username:password pairs from provide input file and it will add<br>
>> username:AESxxxxx value in pool_passwod file.<br>
>><br>
>> Attached contains:<br>
>> 1. Implementation of feature in pg_enc tool<br>
>> 2. Documentation update<br>
>><br>
>> Regards<br>
>> Umar Hayat<br>
>> Principle Software Engineer<br>
>> EnterpriseDB: <a href="https://www.enterpriseDB.com" rel="noreferrer" target="_blank">https://www.enterpriseDB.com</a><br>
>><br>
>><br>
>><br>
</blockquote></div></div>