Re: Change atoi to strtol in same place

On Mon, Oct 7, 2019 at 10:40 AM David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>
> On Mon, 7 Oct 2019 at 13:21, Joe Nelson <joe(at)begriffs(dot)com> wrote:
> >
> > Ashutosh Sharma wrote:
> > > One suggestion: The start value for port number is set to 1, however
> > > it seems like the port number that falls in the range of 1-1023 is
> > > reserved and can't be used. So, is it possible to have the start value
> > > as 1024 instead of 1 ?
> >
> > Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
> > so the range can be updated in one place for all utilities.
>
> (I've only had a very quick look at this, and FWIW, here's my opinion)
>
> It's not for this patch to decide what ports clients can connect to
> PostgreSQL on. As far as I'm aware Windows has no restrictions on what
> ports unprivileged users can listen on. I think we're likely going to
> upset a handful of people if we block the client tools from connecting
> to ports < 1024.
>

AFAIU from the information given in the wiki page -[1], the port
numbers in the range of 1-1023 are for the standard protocols and
services. And there is nowhere mentioned that it is only true for some
OS and not for others. But, having said that I've just verified it on
Linux so I'm not aware of the behaviour on Windows.

[1] - https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers

> > > Further, I encountered one syntax error (INT_MAX undeclared) as the
> > > header file "limits.h" has not been included in postgres_fe.h or
> > > option.h
> >
> > Oops. Added limits.h now in option.h. The Postgres build accidentally
> > worked on my system without explicitly including this header because
> > __has_builtin(__builtin_isinf) is true for me so src/include/port.h
> > pulled in math.h with an #if which pulled in limits.h.
> >
> > > Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> > > > The wording is a bit strange. How about something like pg_standy:
> > > > invalid argument to -k: %s
> >
> > I updated the error messages in pg_standby.
> >
> > > > > *error = psprintf(_("could not parse '%s' as integer"), str);
> > > >
> > > > ... except that they would rather be more explicit about what the
> > > > problem is. "insufficient digits" or "extraneous character", etc.
>
> This part seems over-complex to me. What's wrong with just returning a
> bool and if the caller gets "false", then just have it emit the error,
> such as:
>
> "compression level must be between %d and %d"
>
> I see Michael's patch is adding this new return type, but really, is
> there a good reason why we need to do something special when the user
> does not pass in an integer?
>
> Current patch:
> $ pg_dump -Z blah
> invalid compression level: could not parse 'blah' as integer
>
> I propose:
> $ pg_dump -Z blah
> compression level must be an integer in range 0..9
>
> This might save a few round trips, e.g the current patch will do:
> $ pg_dump -Z blah
> invalid compression level: could not parse 'blah' as integer
> $ pg_dump -Z 12345
> invalid compression level: 12345 is outside range 0..9
> $ ...
>
> Also:
>
> + case PG_STRTOINT_RANGE_ERROR:
> + *error = psprintf(_("%s is outside range "
> + INT64_FORMAT ".." INT64_FORMAT),
> + str, min, max);
>
> The translation string here must be consistent over all platforms. I
> think this will cause issues if the translation string uses %ld and
> the platform requires %lld?
>
> I think what this patch should be really aiming for is to simplify the
> client command-line argument parsing and adding what benefits it can.
> I don't think there's really a need to make anything more complex than
> it is already here.
>
> I think you should maybe aim for 2 patches here.
>
> Patch 1: Add new function to validate range and return bool indicating
> if the string is an integer within range. Set output argument to the
> int value if it is valid. Modify all locations where we currently
> validate the range of the input arg to use the new function.
>
> Patch 2: Add additional validation where we don't currently do
> anything. e.g pg_dump -j
>
> We can then see if there's any merit in patch 2 of if it's adding more
> complexity than is really needed.
>
> I also think some compilers won't like:
>
> + compressLevel = parsed;
>
> given that "parsed" is int64 and "compressLevel" is int, surely some
> compilers will warn of possible truncation? An explicit cast to int
> should fix those or you could consider just writing a version of the
> function for int32 and int64 and directly passing in the variable to
> be set.
>
> --
> David Rowley http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services