[pgpool-hackers: 3556] Re: [PATCH] Feature: Add support for an user/password input file to pg_enc

[Tatsuo Ishii]

Thanks! Patch committed/pushed along with Japanese document by me.

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