From: | Hamid Akhtar <hamid(dot)akhtar(at)gmail(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
Subject: | Re: Minor issues in .pgpass |
Date: | 2020-03-03 12:38:14 |
Message-ID: | CANugjht2gK241gm_ET+_kO2tFf9hDzY=uLFJqG9yUEbE1tVgBQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>
wrote:
>
>
> On 2020/02/29 0:46, Hamid Akhtar wrote:
> > The following review has been posted through the commitfest application:
> > make installcheck-world: not tested
> > Implements feature: not tested
> > Spec compliant: not tested
> > Documentation: not tested
> >
> > First of all, this seems like fixing a valid issue, albeit, the
> probability of somebody messing is low, but it is still better to fix this
> problem.
> >
> > I've not tested the patch in any detail, however, there are a couple of
> comments I have before I proceed on with detailed testing.
>
> Thanks for the review and comments!
>
> > 1. pgindent is showing a few issues with formatting. Please have a look
> and resolve those.
>
> Yes.
>
> > 2. I think you can potentially use "len" variable instead of introducing
> "buflen" and "tmplen" variables.
>
> Basically I don't want to use the same variable for several purposes
> because which would decrease the code readability.
>
> > Also, I would choose a more appropriate name for "tmp" variable.
>
> Yeah, so what about "rest" as the variable name?
>
> > I believe if you move the following lines before the conditional
> statement and simply and change the if statement to "if (len >= sizeof(buf)
> - 1)", it will serve the purpose.
>
> ISTM that this doesn't work correctly when the "buf" contains
> trailing carriage returns but not newlines (i.e., this line is too long
> so the "buf" doesn't include newline). In this case, pg_strip_crlf()
> shorten the "buf" and then its return value "len" should become
> less than sizeof(buf). So the following condition always becomes
> false unexpectedly in that case even though there is still rest of
> the line to eat.
>
Per code comments for pg_strip_crlf:
"pg_strip_crlf -- Remove any trailing newline and carriage return"
If the buf read contains a newline or a carriage return at the end, then
clearly the line
is not exceeding the sizeof(buf). If alternatively, it doesn't, then
pg_strip_crlf will have
no effect on string length and for any lines exceeding sizeof(buf), the
following conditional
statement becomes true.
> > + if (len >= sizeof(buf) - 1)
> > + {
> > + char tmp[LINELEN];
>
> Regards,
>
> --
> Fujii Masao
> NTT DATA CORPORATION
> Advanced Platform Technology Group
> Research and Development Headquarters
>
--
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL:+923335449950 EMAIL: mailto:hamid(dot)akhtar(at)highgo(dot)ca
SKYPE: engineeredvirus
From | Date | Subject | |
---|---|---|---|
Next Message | David Steele | 2020-03-03 13:02:51 | Re: [PATCH] Replica sends an incorrect epoch in its hot standby feedback to the Master |
Previous Message | tushar | 2020-03-03 10:34:07 | Re: backup manifests |