Обсуждение: Redefining inet_net_ntop

On 1/25/18 22:24, Craig Ringer wrote:
> port.h declares inet_net_ntop and we always compile our own
> from port/inet_net_ntop.c .
> 
> But it's part of -lresolv on Linux, and more importantly, it's declared
> in <inet/arpa.h>.
> 
> Should we be using our own if the OS has it? I'm thinking of adding a
> test to configure and omitting our own version if configure finds it.

ISTR that ours is hacked to produce specific output.  Do the regression
tests still pass if you use the one from the OS?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Craig Ringer <craig@2ndquadrant.com> writes:
> Should we be using our own if the OS has it? I'm thinking of adding a test
> to configure and omitting our own version if configure finds it. Objections?

I can't imagine that there's any real upside here.  The amount of code
involved is barely over a kilobyte, and we'd be exposing ourselves to
indeterminate version discrepancies.

Having said that, we got that code from bind, and the release process docs
suggest that we should check for upstream changes every so often.  I don't
think we've done so in a long time :-(

            regards, tom lane

> port.h declares inet_net_ntop and we always compile our own from
> port/inet_net_ntop.c .

There is another copy of it under backend/utils/adt/inet_cidr_ntop.c.
The code looks different but does 90% the same thing.  Their naming
and usage is confusing.

I recently needed to format IP addresses as DNS PTR records in the
database, and got annoyed by having no functions that outputs IPv6
addresses in easily parseable format like
0000:0000:0000:0000:0000:0000:0000:0000.  I was going to send a patch
to unify those C functions and add another SQL function to get
addresses in such format.  Is this a good plan?  Where should those C
functions be on the tree if they are not port of anything anymore?

Emre Hasegeli <emre@hasegeli.com> writes:
>> port.h declares inet_net_ntop and we always compile our own from
>> port/inet_net_ntop.c .

> There is another copy of it under backend/utils/adt/inet_cidr_ntop.c.
> The code looks different but does 90% the same thing.  Their naming
> and usage is confusing.

> I recently needed to format IP addresses as DNS PTR records in the
> database, and got annoyed by having no functions that outputs IPv6
> addresses in easily parseable format like
> 0000:0000:0000:0000:0000:0000:0000:0000.  I was going to send a patch
> to unify those C functions and add another SQL function to get
> addresses in such format.  Is this a good plan?  Where should those C
> functions be on the tree if they are not port of anything anymore?

Almost certainly, the thing to do is absorb updated code from bind,
not roll our own.

            regards, tom lane

Craig Ringer <craig@2ndquadrant.com> writes:
> On 27 January 2018 at 04:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Almost certainly, the thing to do is absorb updated code from bind,
>> not roll our own.

> I asked because I didn't see any comments explaining why we had it and why
> we built it even when the local system has support for it.

> I noticed because I was building an extension in C++ (yeah, fun) and it
> breaks because <inet/arpa.h>'s definition of inet_net_ntop is annotated
> with _THROW , which expands to throw() when building in c++. But this makes
> the prototype incompatible with the one we (re)declare in port.h without
> _THROW and causes  #include "postgres.h" to fail.

Hm ... kinda proves my point about the local-system version not being
entirely stable :-(

> Sure, I can add a hack to c.h to define _THROW as a no-op when not on glibc
> and all that, assuming I get far enough with this extension to bother. But
> it made me ask why we have this duplication in the first place, hence this
> post.

In some other cases, we take the trouble to notice whether the system
headers provide a declaration for some function, and omit redeclaring
the function in port.h if so.  That would be a cleaner answer than
trying to muck with _THROW, so far as the header is concerned, but
I'm not sure whether you'd still get an error when compiling
src/port/inet_net_ntop.c.

Another choice would be to stick a pg_ prefix on the function name.

            regards, tom lane