Opened 9 years ago

Closed 9 years ago

#353 closed enhancement (fixed)

Move function 'check_port()' and 'check_addr()' to one library

Reported by: zhanglikun Owned by: vorner
Priority: medium Milestone:
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Currently, the definition of check_port() and check_addr() can be found in bind10.py, xfrout.py, xfrin.py, cmdctl.py, they should be removed to one library, and avoid the duplicated function definition.

Subtickets

Change History (10)

comment:1 Changed 9 years ago by vorner

  • Owner set to vorner
  • Status changed from new to assigned

comment:2 Changed 9 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from assigned to reviewing

I moved them to the isc.net.check module, and I created isc.net.addr on the way as well.

I didn't find any use of these functions in xfrout.

The proposed changelog entry is:

[func]             Michal Vaner
Added isc.net.check module to check ip addresses and ports for correctness
and isc.net.addr to hold IP address. The bind10, xfrin and cmdctl programs
are modified to use it.
(Trac #353, svn rTBD)

comment:3 Changed 9 years ago by vorner

I forgot to add, it is in branch branches/trac353, branchpoint is r3067 and head is r3087.

comment:4 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:5 follow-up: Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

I've not seen anything obviously wrong, but one test failed for me:

FAIL: test_fail (__main__.TestCheckIP)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jinmei/src/isc/git/bind10-trac353/src/lib/python/isc/net/tests/check_test.py", line 53, in test_fail
    self.assertRaises(ValueError, addr_check, "0000.0.0.0")
AssertionError: ValueError not raised by addr_check

This is strange. Maybe the inet_ntop() implementation on MacOS X (it's BSD-derived as far as I know) is buggy. But we'll have to work around it.

(All other tests passed)

I also have following minor comments. I'd leave it to you whether and how to address these points.

  • port_check()/addr_check() may not be a good name (and in that sense check.py, too) because xxx_check() sounds like performing a check only and it's not intuitive for it to return something (other than OK/NG). But I also saw these functions are sometimes used just for checking, so maybe it's okay as a compromise.
  • it's not clear that IPAddr() raises socket.error against an invalid input (unless you read the source code, and has knowledge that inet_pton() raises that exception on failure). Please at least describe it in the function description, and it's probably better to use a user-defined specific exception.
  • in check_test.py, you assume specific output of inet_ntop(), but as far as I know it's implementation dependent (although we may expect a "canonical form" with future implementation due to RFC5952).

comment:6 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Replying to jinmei:

I've not seen anything obviously wrong, but one test failed for me:
[...]
This is strange. Maybe the inet_ntop() implementation on MacOS X (it's BSD-derived as far as I know) is buggy. But we'll have to work around it.

I modified the function to guess first by a regular expression which version of address it is. This saves one call to inet_ntop() that fails in case of IPv6 address and should workaround about this problem as a sideeffect. Could you run the tests once more and see if it really does? I don't have MacOS.

I also have following minor comments. I'd leave it to you whether and how to address these points.

  • port_check()/addr_check() may not be a good name (and in that sense check.py, too) because xxx_check() sounds like performing a check only and it's not intuitive for it to return something

I changed it to _parse, which can be expected to complain if it is invalid. Does it seem better?

  • it's not clear that IPAddr() raises socket.error against an invalid input (unless you read the source code, and has knowledge that inet_pton() raises that exception on failure). Please at least describe it in the function description, and it's probably better to use a user-defined specific exception.

Added and documented.

  • in check_test.py, you assume specific output of inet_ntop(), but as far as I know it's implementation dependent (although we may expect a "canonical form" with future implementation due to RFC5952).

I left it there. For one, I do not see any simple way to check it works. For another, even if it is implementation dependent, the canonical form (which I hope I put there) seems the only reasonable thing it could return and I guess there's no system that would return something else. If there is, the only thing that happens would be a false positive of the test (it would fail even when it shouldn't), which is better than the other way around. Would you mind if it is left there until we actually find a system that returns something else?

comment:7 in reply to: ↑ 6 ; follow-up: Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

Replying to vorner:

I modified the function to guess first by a regular expression which version of address it is. This saves one call to inet_ntop() that fails in case of IPv6 address and should workaround about this problem as a sideeffect. Could you run the tests once more and see if it really does? I don't have MacOS.

The offending test now passes on MacOS X.

Understanding it may be controversial, however, I always wonder why some people tend to write in-house versions of this type of parser (e.g., by using a regular expression) while the standard getaddrinfo() library is provided exactly for this purpose.

Regular expressions are often difficult to understand for many people, which is in itself against one major goal of BIND 10: better code understandability. And, in general, in-house code tends to more errors than carefully written and widely deployed standard libraries (which is why I believe the "NIH syndrome" is considered a bad practice). In fact, the regular expression "isv6" has a minor bug: it cannot recognize the third form of IPv6 textual representation as defined in Section 2.2 of RFC4291: It incorrectly rejects forms like "::ffff:192.0.2.1" or "2001:4f8:3:bb::0.0.0.5" (yes, the latter form is a valid representation even though it's quite unlikely to see the latter form in practice).

Admittedly, standard libraries are not always correct (just like we saw it in OS X's inet_pton()), but if we use it as an excuse to introduce an in-house partial parser, we should end up even replacing inet_pton() with an in-house complete parser.

So, my suggestion is:

  • ignore the buggy behavior of MacOS. it should be a minor and rare error anyway, so IMO it's not worth making our implementation more complicated and less understandable. If we want to ensure this type of bogus string be rejected in other platforms, I'd rather move the complexity to the test code, e.g.:
            try:
                # XXX: MacOS X's inet_pton() doesn't reject this form, so we
                # check the behavior of the underlying library implementation
                # before the actual test
                socket.inet_pton(socket.AF_INET, "0000.0.0.0")
            except socket.error:
                self.assertRaises(ValueError, addr_parse, "0000.0.0.0")
    
  • then, independently from how to address the buggy inet_pton() behavior, if we want to save the unnecessary call to inet_pton() for IPv6, use socket.getaddrinfo(), e.g.:
            try:
                addrinfo = socket.getaddrinfo(addr, None)[0]
                self.family = addrinfo[0]
                # (we could avoid the call to inet_pton() if it's okay to just
                # hold the string address or address tuple)
                self.addr = socket.inet_pton(self.family, addrinfo[4][0])
            except socket.error as e:
                raise InvalidAddress(str(e))
    

I also have following minor comments. I'd leave it to you whether and how to address these points.

  • port_check()/addr_check() may not be a good name (and in that sense check.py, too) because xxx_check() sounds like performing a check only and it's not intuitive for it to return something

I changed it to _parse, which can be expected to complain if it is invalid. Does it seem better?

Yes, I think so.

  • it's not clear that IPAddr() raises socket.error against an invalid input (unless you read the source code, and has knowledge that inet_pton() raises that exception on failure). Please at least describe it in the function description, and it's probably better to use a user-defined specific exception.

Added and documented.

Confirmed.

  • in check_test.py, you assume specific output of inet_ntop(), but as far as I know it's implementation dependent (although we may expect a "canonical form" with future implementation due to RFC5952).

I left it there. For one, I do not see any simple way to check it works. For another, even if it is implementation dependent, the canonical form (which I hope I put there) seems the only reasonable thing it could return and I guess there's no system that would return something else. If there is, the only thing that happens would be a false positive of the test (it would fail even when it shouldn't), which is better than the other way around. Would you mind if it is left there until we actually find a system that returns something else?

That's fine for me, but I'd suggest commenting on it in the test code.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Replying to jinmei:

Understanding it may be controversial, however, I always wonder why some people tend to write in-house versions of this type of parser (e.g., by using a regular expression) while the standard getaddrinfo() library is provided exactly for this purpose.

Well, because people tend not to remember library functions and regular expression is easy way to do it. I did it according to your suggestion, just added flag to disable name resolution.

That's fine for me, but I'd suggest commenting on it in the test code.

Done, hopefully it is readable.

What about now?

comment:9 in reply to: ↑ 8 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

Replying to vorner:

Replying to jinmei:

Understanding it may be controversial, however, I always wonder why some people tend to write in-house versions of this type of parser (e.g., by using a regular expression) while the standard getaddrinfo() library is provided exactly for this purpose.

Well, because people tend not to remember library functions and regular expression is easy way to do it. I did it according to your suggestion, just added flag to disable name resolution.

Looks good, and I'm impressive to see you even included socket.AI_NUMERICHOST, which I forgot to mention and planned to note in this response:-)

That's fine for me, but I'd suggest commenting on it in the test code.

Done, hopefully it is readable.

What about now?

Splendid. Please go merge it to trunk.

comment:10 Changed 9 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks, merged, closing.

Note: See TracTickets for help on using tickets.