Обсуждение: pgsql: Define INADDR_NONE on Solaris when it's missing.

Log Message:
-----------
Define INADDR_NONE on Solaris when it's missing. Per a couple of buildfarm
members complaining.

Modified Files:
--------------
    pgsql/src/include/port:
        solaris.h (r1.17 -> r1.18)
        (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/port/solaris.h?r1=1.17&r2=1.18)

mha@postgresql.org (Magnus Hagander) writes:
> Log Message:
> -----------
> Define INADDR_NONE on Solaris when it's missing. Per a couple of buildfarm
> members complaining.

This seems likely to break as much as it fixes, since there's no very
good reason to assume that whatever header should define INADDR_NONE
has been included before the os.h header file has been read.

Possibly more to the point, where are we using INADDR_NONE anyway?

            regards, tom lane

On Thu, Jan 28, 2010 at 16:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> mha@postgresql.org (Magnus Hagander) writes:
>> Log Message:
>> -----------
>> Define INADDR_NONE on Solaris when it's missing. Per a couple of buildfarm
>> members complaining.
>
> This seems likely to break as much as it fixes, since there's no very
> good reason to assume that whatever header should define INADDR_NONE
> has been included before the os.h header file has been read.

Hmm. Where would you suggest it goes?

The addition of such a define is in a lot of places on the net as
fixing just this issue, and was also recommended by Zdenek as the fix
for Solaris. But I can agree it may be in the wrong place :-)

> Possibly more to the point, where are we using INADDR_NONE anyway?

In the RADIUS code.


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

Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Jan 28, 2010 at 16:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Possibly more to the point, where are we using INADDR_NONE anyway?

> In the RADIUS code.

Oh, that's why it isn't in my tree and has zero portability track record ...

I think what this shows is we should look for a way to avoid using
INADDR_NONE.  What's your grounds for believing it's portable at all?
In the Single Unix Spec I only see INADDR_ANY and INADDR_BROADCAST
defined.

            regards, tom lane

On Thu, Jan 28, 2010 at 17:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Thu, Jan 28, 2010 at 16:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Possibly more to the point, where are we using INADDR_NONE anyway?
>
>> In the RADIUS code.
>
> Oh, that's why it isn't in my tree and has zero portability track record ...
>
> I think what this shows is we should look for a way to avoid using
> INADDR_NONE.  What's your grounds for believing it's portable at all?
> In the Single Unix Spec I only see INADDR_ANY and INADDR_BROADCAST
> defined.

Um, I don't think I have any specific grounds for it, other than
having seen it in a lot of other software :-)

From some more googling
(http://www.opengroup.org/onlinepubs/000095399/functions/inet_addr.html),
it says it will return (in_addr_t)(-1), though, so maybe we should
just move that #ifdef out to some global place?


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

Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Jan 28, 2010 at 17:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think what this shows is we should look for a way to avoid using
>> INADDR_NONE.

>> From some more googling
> (http://www.opengroup.org/onlinepubs/000095399/functions/inet_addr.html),
> it says it will return (in_addr_t)(-1), though, so maybe we should
> just move that #ifdef out to some global place?

Given the way that's written, I think we should just compare the result
to (in_addr_t)(-1), and not assume there's any macro provided for that.

However, now that I know the real issue is you're using inet_addr, I
would like to know why you're not using inet_aton instead; or even
better, something that also copes with IPv6.

            regards, tom lane

On Thu, Jan 28, 2010 at 17:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Thu, Jan 28, 2010 at 17:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think what this shows is we should look for a way to avoid using
>>> INADDR_NONE.
>
>>> From some more googling
>> (http://www.opengroup.org/onlinepubs/000095399/functions/inet_addr.html),
>> it says it will return (in_addr_t)(-1), though, so maybe we should
>> just move that #ifdef out to some global place?
>
> Given the way that's written, I think we should just compare the result
> to (in_addr_t)(-1), and not assume there's any macro provided for that.

Well, that doesn't match all other platforms..


> However, now that I know the real issue is you're using inet_addr, I
> would like to know why you're not using inet_aton instead; or even
> better, something that also copes with IPv6.

"Path of least resistance?"

Which method would you suggest?


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

Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Jan 28, 2010 at 17:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However, now that I know the real issue is you're using inet_addr, I
>> would like to know why you're not using inet_aton instead; or even
>> better, something that also copes with IPv6.

> "Path of least resistance?"

> Which method would you suggest?

I haven't actually read the RADIUS patch, but generally we rely on
pg_getaddrinfo_all to interpret strings representing IP addresses.
Is there a reason not to use that?

            regards, tom lane

On Thu, Jan 28, 2010 at 21:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Thu, Jan 28, 2010 at 17:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> However, now that I know the real issue is you're using inet_addr, I
>>> would like to know why you're not using inet_aton instead; or even
>>> better, something that also copes with IPv6.
>
>> "Path of least resistance?"
>
>> Which method would you suggest?
>
> I haven't actually read the RADIUS patch, but generally we rely on
> pg_getaddrinfo_all to interpret strings representing IP addresses.
> Is there a reason not to use that?

I don't think so. I'll look it over.


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