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

On Tue, Mar 27, 2018 at 3:03 PM, David G. Johnston <
david(dot)g(dot)johnston(at)gmail(dot)com> wrote:

> On Mon, Mar 26, 2018 at 8:24 PM, Michael Paquier <michael(at)paquier(dot)xyz>
> wrote:
>
>> On Tue, Mar 27, 2018 at 11:43:27AM +1100, Haribabu Kommi wrote:
>> > Patch attached with the above behavior along with other comments from
>> > upthread.
>>
>> Thanks for the updated version.
>>
>> The function changes look logically good to me.
>>
>> + <para>
>> + The <function>PQhost</function> function returns NULL when the
>> + input conn parameter is NULL or an empty string if conn cannot be
>> evaluated.
>> + Applications of this function must carefully evaluate the return
>> value.
>> + </para>
>>
>> - "Applications of this function must carefully evaluate the return
>> value" is rather vague, so I would append to the end "depending on the
>> state of the connection involved."
>> The same applies to PQport() for consistency.
>>
>> Perhaps the documentation should mention as well that making the
>> difference between those different values is particularly relevant when
>> using multiple-value strings? I would rather add one paragraph on the
>> matter than nothing. I really think that we have been lacking clarity
>> in the documentation for those APIs for too long, and people always
>> argue about what they should do. If we have a base documented, then we
>> can more easily argue for the future as well, and things are clear to
>> the user.
>>
> ​
>
> "depending on the state of the connection" doesn't move the goal-posts
> that far though...and "Applications of this function" would be better
> written as "Callers of this function" if left in place.
>
> In any case something like the following framework seems more useful to
> the reader who doesn't want to scan the source code for the PQhost/PQport
> functions.
>
> The PQhost function returns NULL when the input conn parameter is NULL or
> an empty string if conn cannot be evaluated. Otherwise, the return value
> depends on the state of the conn: specifically (translate code to
> documentation here). Furthermore, if both host and hostaddr properties
> exist on conn the return value will contain only the host.
>
> I'm undecided on the need for a <note> element but would lean against it
> in favor of the above, slightly longer, paragraph.
>
> And yes, while I'm not sure right now what the multi-value condition logic
> results in it should be mentioned - at least if the goal of the docs is to
> be a sufficient resource for using these functions. In particular what
> happens when the connection is inactive (unless that falls under "cannot be
> evaluated"...).
>

Thanks for the review.

updated patch attached with additional doc updates as per the suggestion
from the upthreads.

Regards,
Hari Babu
Fujitsu Australia