Porting utility getaddrinfo from NetBSD
ClosedPublic

Authored by lohithbsd_gmail.com on Jan 28 2017, 5:47 AM.

Details

Summary

This revision is to port useful utility getaddrinfo from netbsd to FreeBSD head

Test Plan

Ported and tested on FreeBSD head

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
lohithbsd_gmail.com retitled this revision from to Porting utility getaddrinfo from NetBSD.Jan 28 2017, 5:47 AM
lohithbsd_gmail.com updated this object.
lohithbsd_gmail.com edited the test plan for this revision. (Show Details)
lohithbsd_gmail.com added a reviewer: hiren.
hiren added a subscriber: network.
hiren accepted this revision.Jan 31 2017, 7:34 PM

I'd appreciate another pair of eyes on this.

This revision is now accepted and ready to land.Jan 31 2017, 7:34 PM

I plan to review and commit this, but I don't have time right now. (Practically all of my FreeBSD time is through work, and I'm way too busy this week.)

ae added a subscriber: ae.Jan 31 2017, 7:50 PM

I'm curious, you plan to add this code into FreeBSD base tree, not into the contrib section. Do we need to have all these #ifdefs in such case? I doubt that we have all such HAVE_XXX macros defined in our environment. Also we already have removed AppleTalk support from the tree.

hiren requested changes to this revision.Jan 31 2017, 7:58 PM
In D9365#194220, @ae wrote:

I'm curious, you plan to add this code into FreeBSD base tree, not into the contrib section. Do we need to have all these #ifdefs in such case? I doubt that we have all such HAVE_XXX macros defined in our environment. Also we already have removed AppleTalk support from the tree.

Good points. Is there any preference one way or another? I see we have $src/lib/libnetbsd so I thought this could live there?

I'll work with @lohithbsd_gmail.com to see how he compiled/tested things and get unneeded macros removed and upload the new patch.

This revision now requires changes to proceed.Jan 31 2017, 7:58 PM
In D9365#194224, @hiren wrote:
In D9365#194220, @ae wrote:

I'm curious, you plan to add this code into FreeBSD base tree, not into the contrib section. Do we need to have all these #ifdefs in such case? I doubt that we have all such HAVE_XXX macros defined in our environment. Also we already have removed AppleTalk support from the tree.

Good points. Is there any preference one way or another? I see we have $src/lib/libnetbsd so I thought this could live there?

I'll work with @lohithbsd_gmail.com to see how he compiled/tested things and get unneeded macros removed and upload the new patch.

Thanks a lot for the review comments. I had removed some of the #ifdefs for compiling, will go through all the #ifdefs and remove whichever is not required and will remove APPLE TALK related code.

wblock added a subscriber: wblock.Feb 3 2017, 4:42 PM
wblock added inline comments.
usr.bin/getaddrinfo/getaddrinfo.1
47 ↗(On Diff #24521)

(For this and following comments, I realize you didn't write this, but will comment on areas that are questionable anyway. It might be nice to relay these upstream even if this does not go in contrib.)

Does "as if" mean it does not use getaddrinfo(1), just is a workalike? This should state clearly exactly what the situation is.

50 ↗(On Diff #24521)

"formats" can be read as a noun, so clarify.

routine, then formats them and prints them to standard output.
52 ↗(On Diff #24521)

s/lines of/lines with/

63 ↗(On Diff #24521)

This whole sentence is redundant and weird. (Some lines in replacement might be too long and need wrapping because of Phabricator's phabulousness.)

Depending on the settings in
.Xr nsswitch.conf 5 ,
.Nm
might query DNS for answers.
However, it is not intended to be a general-purpose DNS query utility.
Use
.Xr drill 1
for that.
73 ↗(On Diff #24521)

s/The following/These/

120 ↗(On Diff #24521)
Protocols are numeric or symbolic as listed in
124 ↗(On Diff #24521)
Services are symbolic or numeric with an optional
127 ↗(On Diff #24521)

Negative logic.

If a service is not specified, a hostname is required.
usr.bin/getaddrinfo/getaddrinfo.c
145 ↗(On Diff #24521)

igor says there is empty whitespace on this line, looks like a couple of tabs.

ae added inline comments.Feb 3 2017, 6:06 PM
usr.bin/getaddrinfo/getaddrinfo.1
47 ↗(On Diff #24521)

No, it uses getaddrinfo(3). This utility prints information that you can obtain with getaddrinfo(3) function in a user-friendly format.

wblock added inline comments.Feb 3 2017, 7:37 PM
usr.bin/getaddrinfo/getaddrinfo.1
47 ↗(On Diff #24521)

That's a much better description!

utility resolves host and service names to socket addresses with
.Xr getaddrinfo 3
and prints them to standard output in a user-friendly format.
lohithbsd_gmail.com marked 11 inline comments as done.Mar 8 2017, 7:40 AM
lohithbsd_gmail.com added inline comments.
usr.bin/getaddrinfo/getaddrinfo.1
47 ↗(On Diff #24521)

Yes. I didn't write this. Thanks for the review comments, have taken care all of them!

Removed the Apple Nettalk related code. And addressed all the MAN page review comments.

ae accepted this revision.Mar 8 2017, 9:50 AM

It seems that the 'R' format character of sockaddr_snprintf() function has become useless. In the rest looks good for me.

vangyzen accepted this revision.Mar 15 2017, 7:01 PM
In D9365#205183, @ae wrote:

It seems that the 'R' format character of sockaddr_snprintf() function has become useless. In the rest looks good for me.

Since this is a library function that might have other callers in the future, I recommend keeping it in order to maintain API compatibility.

This revision was automatically updated to reflect the committed changes.