Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types

On Mon, Mar 26, 2018 at 6:34 PM, Kyotaro HORIGUCHI <
horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:

> Hello.
>
> At Mon, 26 Mar 2018 17:49:22 +1100, Haribabu Kommi <
> kommi(dot)haribabu(at)gmail(dot)com> wrote in <CAJrrPGdYQ92R1hNArCByu+gN_
> SGsFMjGvbErjH0N9W8Ry24CxA(at)mail(dot)gmail(dot)com>
>
>
Thanks for the review.

> + The <function>PQhost</function> function returns NULL when the
> + connection is not established, or returns an empty string when
> status
> + of the connection is not <literal>CONNECTION_OK</literal>.
>
> This may be wrong. NULL is only for the case conn == NULL and ""
> for other connection failure. I'm not sure how to express the OOM
> case in the documentation but we could reuturn "" for the
> conn==NULL case if we don't want to distinguish the state in
> PQhost and its family.
>

All the other PQ* functions checks and return NULL when conn==NULL,
returning NULL here may not be good?

And if we are not going to change the above, then PQhost() function
returns 3 values,
- NULL when the conn==NULL
- Actual host or hostaddr of the active connection
- Empty string when the conn is not able to evaluate.

Is it fine to proceed like above?

> > > > My opinion is to add a description that is saying like "these
> > > > functions return unreliable values for a failed connection".
> > >
> > > At the same time I don't think that this is sufficient either, because
> > > for multiple hosts, PQhost() just returns the first one, which makes
> > > absolutely no sense because the value is wrong. So I think that using
> a
> > > third, separate value has some advantages:
> > > - If NULL, this just means that the initialization did not happen.
> > > - If using an empty string, then the value cannot be evaluated.
> > > - If this returns a host or hostaddr (if host has not been specified),
> > > then that's the host which is actually used for the connection.
> > > Having those three states has value for applications in my opinion.
> > >
> > > The same can apply to PQport, or any other functions which for whatever
> > > reason add support for multiple values like host, hostaddr or port.
> > >
> >
> > I hope that I updated the documentation properly to explain all the above
> > cases.
>
> I'm not sure but I'm afraid that some authentication methods
> requires that PQhost() returns a host name for the states other
> than CONNECTION_OK, perhaps CONNECTION_MADE, AWAITING_RESPONSE
> and so, which happens after a connection is established. Even
> without considering that, we can return a sane value after raw
> connection (not a PGconn) is established.
>

Yes, PQhost() function is used even when the connection is fully
established,
so we cannot use the status to return the host name. So the logic of
verifying the
status needs to be removed.

> > > >> Updated patch attached with behavior of returning NULL for
> connections
> > > of
> > > >> CONNECTION_BAD status.
> > > >
> > > > The patch does Assert() in PQhost. I suppose that Assert() in
> > > > client library is usable only when no more (library's) operation
> > > > cannot continue. It would be better to return a fallback value in
> > > > this criteria.
> > >
> > > The patch has to return a value as well. A shaped the patch causes
> > > again compilation warnings because not all code paths have a return
> > > value. So my previous remark has not been fixed. Hari, what do you
> use
> > > as compiler, my gcc blows a warning and reading the patch that's
> > > obviously incorrect.
> > >
> >
> > In my assert enabled build, I didn't get any warning. Yes that patch to
> fix
> > the
> > warning is clearly wrong. I corrected in a different way of adding Assert
> > checks
> > for the hostaddr, because definitely host or hostaddr must be present.
>
> As I wrote upthread, the assertion doesn't seem to be needed. I
> think that a library should allow callers to decide how to handle
> error cases if it is possible.
>

OK. Will correct it.

> > > + PQHost returns NULL when the connection is not established
> or
> > > In the docs, this is wrong for two reasons:
> > > - There should be a <function> markup.
> > > - The name of the function is PQhost, not PGHost.
> > >
> >
> > Corrected.
> >
> > Updated patch attached.
>
> The documentation is written as the following.
>
> - Returns the server host name of the connection.
> + Returns the server host name or host address of the active
> connection.
> This can be a host name, an IP address, or a directory path if the
>
> Is the replacement is required? The following line is stating the
> same thing including the local-socket case.

Ok, will update it to just include the active connection.

Regards,
Hari Babu
Fujitsu Australia