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

On 27 January 2018 at 04:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

Definitely.

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.

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.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

On 29 January 2018 at 17:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

That, plus a comment, seems just fine to me.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services