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

[Umar Hayat]

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
> >>
> >>
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200317/582ea666/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pg_enc_input_file_17Mar20.diff
Type: application/octet-stream
Size: 9453 bytes
Desc: not available
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20200317/582ea666/attachment.obj>