Re: pg_hba.conf: samehost and samenet [REVIEW]

On Sun, Sep 20, 2009 at 05:59, Abhijit Menon-Sen <ams(at)toroid(dot)org> wrote:
> I think the patch is more or less ready, but I have a few minor
> comments:
>
> First, it needs to be reformatted to not use a space before the opening
> parentheses in (some) function calls and definitions.
>
>> *** a/doc/src/sgml/client-auth.sgml
>> --- b/doc/src/sgml/client-auth.sgml
>> [...]
>>
>> +       <para>Instead of an <replaceable>CIDR-address</replaceable>, you can specify
>> +        the values <literal>samehost</literal> or <literal>samenet</literal>. To
>> +        match any address on the subnets connected to the local machine, specify
>> +        <literal>samenet</literal>. By specifying <literal>samehost</literal>, any
>> +        addresses present on the network interfaces of local machine will match.
>> +       </para>
>> +
>
> I'd suggest something like the following instead:
>
>    <para>Instead of a <replaceable>CIDR-address</replaceable>, you can
>    specify <literal>samehost</literal> to match any of the server's own
>    IP addresses, or <literal>samenet</literal> to match any address in
>    a subnet that the server belongs to.
>
>> *** a/src/backend/libpq/hba.c
>> --- b/src/backend/libpq/hba.c
>> [...]
>>
>> +     else if (addr->sa_family == AF_INET &&
>> +                      raddr->addr.ss_family == AF_INET6)
>> +     {
>> +             /*
>> +              * Wrong address family.  We allow only one case: if the file
>> +              * has IPv4 and the port is IPv6, promote the file address to
>> +              * IPv6 and try to match that way.
>> +              */
>
> How about this instead:
>
>    If we're listening on IPv6 but the file specifies an IPv4 address to
>    match against, we promote the latter also to an IPv6 address before
>    trying to match the client's address.
>
> (The comment is repeated elsewhere.)

That's actually a copy/paste from the code that's in hba.c now. That's
not reason not to fix it of course :-)

>> + int
>> + pg_foreach_ifaddr(PgIfAddrCallback callback, void * cb_data)
>> + {
>> + #ifdef WIN32
>> +     return foreach_ifaddr_win32(callback, cb_data);
>> + #else /* !WIN32 */
>> + #ifdef HAVE_GETIFADDRS
>> +     return foreach_ifaddr_getifaddrs(callback, cb_data);
>> + #else /* !HAVE_GETIFADDRS */
>> +     return foreach_ifaddr_ifconf(callback, cb_data);
>> + #endif
>> + #endif /* !WIN32 */
>> + }
>
> First, writing it this way is less noisy:
>
>    #ifdef WIN32
>        return foreach_ifaddr_win32(callback, cb_data);
>    #elif defined(HAVE_GETIFADDRS)
>        return foreach_ifaddr_getifaddrs(callback, cb_data);
>    #else
>        return foreach_ifaddr_ifconf(callback, cb_data);
>    #endif
>
> Second, I can't see that it makes very much difference, but why do it
> this way at all? You could just have each of the three #ifdef blocks
> define a function named pg_foreach_ifaddr() and be done with it. No
> need for a fourth function.

That was my thought as well when I looked at the patch. It'd be
clearer and I think more in line with what we do at other places.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/