On Tue, Mar 3, 2020 at 5:38 PM Hamid Akhtar <hamid.akh...@gmail.com> wrote:
> > > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fu...@oss.nttdata.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. >> > That is fine. > >> > Also, I would choose a more appropriate name for "tmp" variable. >> >> Yeah, so what about "rest" as the variable name? >> > May be something like "excess_buf" or any other one that describes that these bytes are to be discarded. > >> > 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.akh...@highgo.ca > SKYPE: engineeredvirus > -- Highgo Software (Canada/China/Pakistan) URL : www.highgo.ca ADDR: 10318 WHALLEY BLVD, Surrey, BC CELL:+923335449950 EMAIL: mailto:hamid.akh...@highgo.ca SKYPE: engineeredvirus