Page MenuHomeFreeBSD

Generate /etc/services from the IANA registry (plus local mods)
Needs ReviewPublic

Authored by vangyzen on Sep 10 2018, 4:59 PM.

Details

Reviewers
asomers
rgrimes
Summary

Generate /etc/services from

https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xml

and a locally maintained local.xml.

This required creating the initial version of local.xml, which
was also automated. (The code for this can be removed when we are
sure it is no longer needed.) All of our current /etc/services names
still exist, although the official name and comment for a given
number/protocol tuple are determined by the IANA registry, so they
might have changed.

/etc/services grows from 2,367 lines and 2,501 names to 11,538 lines
and 12,005 names. /var/db/services.db grows from 2 MB to 4.2 MB.

Test Plan

I manually read through the generated file. It looks reasonable.

I parsed the file with services_count.c. It parsed 12,005 names,
the same number printed by services_parser.py when creating the file.

I ran services_mkdb -q. It seemed happy.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint ErrorsExcuse: .
SeverityLocationCodeMessage
Errorusr.sbin/services_mkdb/services_parser.py:30E265PEP8 E265
Errorusr.sbin/services_mkdb/services_parser.py:35E302PEP8 E302
Errorusr.sbin/services_mkdb/services_parser.py:42E301PEP8 E301
Errorusr.sbin/services_mkdb/services_parser.py:44E301PEP8 E301
Errorusr.sbin/services_mkdb/services_parser.py:46E301PEP8 E301
Errorusr.sbin/services_mkdb/services_parser.py:48E301PEP8 E301
Errorusr.sbin/services_mkdb/services_parser.py:50E301PEP8 E301
Errorusr.sbin/services_mkdb/services_parser.py:52E301PEP8 E301
Errorusr.sbin/services_mkdb/services_parser.py:54E301PEP8 E301
Errorusr.sbin/services_mkdb/services_parser.py:56E301PEP8 E301
Errorusr.sbin/services_mkdb/services_parser.py:74E302PEP8 E302
Errorusr.sbin/services_mkdb/services_parser.py:81E501PEP8 E501
Errorusr.sbin/services_mkdb/services_parser.py:95E302PEP8 E302
Errorusr.sbin/services_mkdb/services_parser.py:97E128PEP8 E128
Errorusr.sbin/services_mkdb/services_parser.py:99E302PEP8 E302
Errorusr.sbin/services_mkdb/services_parser.py:145E128PEP8 E128
Errorusr.sbin/services_mkdb/services_parser.py:152E302PEP8 E302
Errorusr.sbin/services_mkdb/services_parser.py:181E302PEP8 E302
Warningusr.sbin/services_mkdb/service-names-port-numbers.xml:8995SPELL1Possible Spelling Mistake
Warningusr.sbin/services_mkdb/service-names-port-numbers.xml:9002SPELL1Possible Spelling Mistake
Warningusr.sbin/services_mkdb/service-names-port-numbers.xml:70857SPELL1Possible Spelling Mistake
Warningusr.sbin/services_mkdb/service-names-port-numbers.xml:70865SPELL1Possible Spelling Mistake
Warningusr.sbin/services_mkdb/service-names-port-numbers.xml:95401SPELL1Possible Spelling Mistake
Warningusr.sbin/services_mkdb/service-names-port-numbers.xml:96488SPELL1Possible Spelling Mistake
Warningusr.sbin/services_mkdb/service-names-port-numbers.xml:102845SPELL1Possible Spelling Mistake
Unit
No Unit Test Coverage
Build Status
Buildable 19498
Build 19091: arc lint + arc unit

Event Timeline

I will address the PEP8 errors.

It's not clear to me what the role of local.xml is. I had assumed from the description that local.xml just contained additional entries (or perhaps overrides for existing entries) relative to the official IANA registry. However, local.xml seems to contain a lot of entries that duplicate the IANA registry?

In D17106#364690, @jhb wrote:

It's not clear to me what the role of local.xml is. I had assumed from the description that local.xml just contained additional entries (or perhaps overrides for existing entries) relative to the official IANA registry. However, local.xml seems to contain a lot of entries that duplicate the IANA registry?

It's exactly what you expected. Can you point me to a duplicate? There shouldn't be any. Note that it adds a lot of less common protocols (ddp, sctp).

asomers added inline comments.
usr.sbin/services_mkdb/services
28

This mixes whitespace changes with content changes. That makes it very hard to see the content changes . Can you please revert the whitespace changes? If you think they're warranted, then you should submit them later in a separate commit.

usr.sbin/services_mkdb/services
28

This file is now generated. To revert the whitespace changes, I would need to modify the script that generates it. I'd rather not do that.

I'm pretty sure you can tell Phabricator to hide changes in whitespace.

usr.sbin/services_mkdb/services
28

Yes, you'll need to modify the script. But it's actually not very hard, because the existing file is pretty consistent. In fact, I've already done it. I'll send you the script I used.

Is there anyway to NOT have the 141,000 line xml version of the file stored in SVN?
Also we need to make absolutely sure that any services used by ports are not effected by anything this changes.

Is there anyway to NOT have the 141,000 line xml version of the file stored in SVN?

I'd like to keep it, but I'll defer to the majority. The IANA is probably reliable enough that we don't need to keep it in our VCS.

Also we need to make absolutely sure that any services used by ports are not effected by anything this changes.

I sincerely doubt an exp-run would show anything, so it's up to users to report bugs.

At the suggestion of @mjg, I tested the performance of the parser in libc by calling getservent() (and envservent() after it returned NULL so I could reparse the file many times). Time increased linearly as the number of lines, and memory usage (RSS) increased less than 1%.

usr.sbin/services_mkdb/services
28

Of course it's not very hard. I just don't want to keep the old whitespace, because it's actually a goofy mixture of spaces and tabs. As an example, compare port 631 to 633. I also don't like the absence of a space after the #.

I would not try to get this into 12.0, due to unpredictable runtime impact.

usr.sbin/services_mkdb/services
28

I agree that the existing whitespace isn't the best. I'm just saying that we can't adequately review whitespace and comment changes at the same time. Nor is it sufficient to hide the whitespace in Phabricator, because that will still mingle whitespace and content changes in the SVN history.

usr.sbin/services_mkdb/services
28

OK. I'll open a second review, containing just this file, without the whitespace changes. When I commit, I'll commit the whitespace change first. How does that sound?

usr.sbin/services_mkdb/services
28

Sure.