add support for compressed IPv4 addresses, like 10/8

[grayed]

This allows the IPv4 addresses with CIDR prefix to be specified and displayed in compressed form, for example:

address       uncompressed      compressed
1.2.3.4/12 -> 1.2.3.4/12     -> 1.2.3.4/12
1.2.3./12  -> 1.2.3.0/12     -> 1.2.3/12
1.2.3/12   -> 1.2.3.0/12     -> 1.2.3/12
10/8       -> 10.0.0.0/8     -> 10/8

This involved full rewrite of IPv4 parser using some sort of finite state machine, making code more simple and compact.

[pbiering]

Thank you for the contribution, but as written, the 2 cases are somehow strange or not unique, I would leave this out - or even better to check the parser mark them as "invalid" in the test case script and check whether parser not accidently interprets that as IPv4 address

[grayed]

Ah, I've likely misunderstood those test lines. I really want is to mark such input as invalid. But I tried to follow original idea in main().

There's an idea: since parsing IPv4 addresses is relatively fast and simple, call addr_to_ipv4addrstruct() directly from main() when trying to detect format, instead of playing games with numdots and friends. FWIW, I'd implement format autodetection as try-fallback of each format one by one, reusing code already there). This way there will be no false positives from autodetection. What do you think?

[pbiering]

Ah, I've likely misunderstood those test lines. I really want is to mark such input as invalid. But I tried to follow original idea in main().

There's an idea: since parsing IPv4 addresses is relatively fast and simple, call addr_to_ipv4addrstruct() directly from main() when trying to detect format, instead of playing games with numdots and friends. FWIW, I'd implement format autodetection as try-fallback of each format one by one, reusing code already there). This way there will be no false positives from autodetection. What do you think?

There are always ways to improve and using the built-in address parser directly in some kind of "dry-run" mode would be for sure an improvement to the current "guessing" code. Feel free to contribute, as long as all configured test cases working well, nothing should be broken.

BTW: bad case tests can be put here: testscenarios_auto_bad() in test_ipv6calc.sh

[grayed]

Ah, I've likely misunderstood those test lines. I really want is to mark such input as invalid. But I tried to follow original idea in main(). There's an idea: since parsing IPv4 addresses is relatively fast and simple, call addr_to_ipv4addrstruct() directly from main() when trying to detect format, instead of playing games with numdots and friends. FWIW, I'd implement format autodetection as try-fallback of each format one by one, reusing code already there). This way there will be no false positives from autodetection. What do you think?

There are always ways to improve and using the built-in address parser directly in some kind of "dry-run" mode would be for sure an improvement to the current "guessing" code. Feel free to contribute, as long as all configured test cases working well, nothing should be broken.

Then I plan polish-finish IPv4 first and then we'll see what can be done with others. Do you agree?

BTW: bad case tests can be put here: testscenarios_auto_bad() in test_ipv6calc.sh

Yep, and I've put some there. :-)

BTW, some shell scripting hint here. :-) If you assign true and false instead of 1 and 0 to $verbose, you can simplify checks:

if $verbose; then
        foo ...
fi

$verbose || bar ...

[pbiering]

There are always ways to improve and using the built-in address parser directly in some kind of "dry-run" mode would be for sure an improvement to the current "guessing" code. Feel free to contribute, as long as all configured test cases working well, nothing should be broken.

Then I plan polish-finish IPv4 first and then we'll see what can be done with others. Do you agree?

Sure.

BTW: bad case tests can be put here: testscenarios_auto_bad() in test_ipv6calc.sh

Yep, and I've put some there. :-)

BTW, some shell scripting hint here. :-) If you assign true and false instead of 1 and 0 to $verbose, you can simplify checks:

if $verbose; then
        foo ...
fi

$verbose || bar ...

Scripting partially started many ago...also with my at that time limited knowledge...then many copy-paste to avoid any breaks...optimization was either not recognized or postponed -> feel free to replace either partially or on bulk, but best if splitted into several PRs for easier reviewing. Thank you!

[grayed]

I tweaked IPv4 parsing code, and now realized that 1.2 and 1.2.3 are now being treated as IPv6 reversed-DNS form.

[pbiering]

I tweaked IPv4 parsing code, and now realized that 1.2 and 1.2.3 are now being treated as IPv6 reversed-DNS form.

hmm, question would be which support should be really added

address    uncompressed   compressed
1.2.3.4 -> 1.2.3.4     -> 1.2.3.4  -> OK, but no change
1.2.3.  -> 1.2.3.0     -> 1.2.3 -> see below
1.2.3   -> 1.2.3.0     -> 1.2.3 -> see below
10/8    -> 10.0.0.0/8  -> 10/8 -> OK

should we really add support of compressed/uncompressed for partial IPv4 addresses without any prefix notation?

Wouldn't be following not enough for autodetection:

address    uncompressed   compressed
1.2.3/24   -> 1.2.3.0/24     -> 1.2.3/24
1.2.3.0/24   -> 1.2.3.0/24  -> 1.2.3/24

"/xx" can be suppressed then by implementing specific output option (--printprefix) (like for IPv6)

In case "ipv4addr" is specified as hardcoded input type, additional support like you mentioned in 1st example can be supported, if needed in real-world.

=> autodetect should potentially not catch all cases which the related input parser finally understands.

[grayed]

I tweaked IPv4 parsing code, and now realized that 1.2 and 1.2.3 are now being treated as IPv6 reversed-DNS form.

hmm, question would be which support should be really added

address    uncompressed   compressed
1.2.3.4 -> 1.2.3.4     -> 1.2.3.4  -> OK, but no change
1.2.3.  -> 1.2.3.0     -> 1.2.3 -> see below
1.2.3   -> 1.2.3.0     -> 1.2.3 -> see below
10/8    -> 10.0.0.0/8  -> 10/8 -> OK

should we really add support of compressed/uncompressed for partial IPv4 addresses without any prefix notation?

Yep, I forgot to tweak the original PR comment to reflect we're only allowing compressed for for CIDR-prefixed addresses. FIxed.

Wouldn't be following not enough for autodetection:

address    uncompressed   compressed
1.2.3/24   -> 1.2.3.0/24     -> 1.2.3/24
1.2.3.0/24   -> 1.2.3.0/24  -> 1.2.3/24

"/xx" can be suppressed then by implementing specific output option (--printprefix) (like for IPv6)

In case "ipv4addr" is specified as hardcoded input type, additional support like you mentioned in 1st example can be supported, if needed in real-world.

Actually, such support would mean removing code, which I additionally added to addr_to_ipv4addrstruct() to disallow non-CIDR compact IPv4 addresses. :-)

=> autodetect should potentially not catch all cases which the related input parser finally understands.

Yes. This is fixable by running autodection code in order, probing most likely variants first. Obviously, IPv4 addresses are more probable than IPv6 reversed-DNS, so the IPv4 check goes earlier. It was so already, though, and it's a question to IPv6 detection code, IMO.

... So let's summarize:

                       Proposed        Release
Addrspec            detect? parse?  detect? parse?  change?
1.2.3.4             ipv4    yes     ipv4    yes     no
1.2.3.              ipv6rev yes     ipv6rev yes     no
1.2.3               ipv6rev yes     ipv6rev yes     no
1.2.                ipv6rev yes     ipv6rev yes     no
1.2                 ipv6rev no      ipv6rev no      no
1.                  ipv6rev no      ipv6rev no      no
1                   no      no      no      no      no
1.2.3.4/0           ipv4    yes     ipv4    yes     no
1.2.3.4/00          ipv4    yes     ipv4    yes     no
1.2.3.4/000         ipv4    yes     ipv4    yes     no
1.2.3.4/00000000000 ipv4    yes     no      no      YES
123.123.123.123/000 ipv4    yes     no      no      YES
1.2.3./0            ipv4    yes     no      no      YES
1.2.3/0             ipv4    yes     no      no      YES
1.2./0              ipv4    yes     no      no      YES
1.2/0               ipv4    yes     no      no      YES
1./0                ipv4    yes     no      no      YES
1/0                 ipv4    yes     no      no      YES

[pbiering]

Do you mean with "Old" your 1st implementation or the current release behavior?

Because currently they are not detected beside others...

1.2.3.4/00000000000 ipv4 yes no no YES 123.123.123.123/000 ipv4 yes no no YES

[grayed]

Do you mean with "Old" your 1st implementation or the current release behavior?

The "master" branch (i.e., current release).

Because currently they are not detected beside others...

1.2.3.4/00000000000 ipv4 yes no no YES 123.123.123.123/000 ipv4 yes no no YES

The /000... examples went to into my head recently, I didn't ever thought about them during code writing.

[pbiering]

Hmm do I misunderstand your table?

compiled here it will not autodetect IPv4 addresses:

[git:ipv6calc:master]$ ./ipv6calc  123.123.123.123/000
no input type specified, try autodetection... Input-type isn't autodetected
[git:ipv6calc:master]$ ./ipv6calc  1.2.3.4/00000000000
no input type specified, try autodetection... Input-type isn't autodetected

[grayed]

Hmm do I misunderstand your table?

Of course, you did. Because I had the columns reversed. :-( Sorry. I fixed the table headers.

[pbiering]

                       Proposed        Release
Addrspec            detect? parse?  detect? parse?  change?
1.2.3.4             ipv4    yes     ipv4    yes     no
1.2.3.              ipv6rev yes     ipv6rev yes     no
1.2.3               ipv6rev yes     ipv6rev yes     no
1.2.                ipv6rev yes     ipv6rev yes     no
1.2                 ipv6rev no      ipv6rev no      no
1.                  ipv6rev no      ipv6rev no      no
1                   no      no      no      no      no
1.2.3.4/0           ipv4    yes     ipv4    yes     no
1.2.3.4/00          ipv4    yes     ipv4    yes     no
1.2.3.4/000         ipv4    yes     ipv4    yes     no (1)
1.2.3.4/00000000000 ipv4    yes     no      no      YES (2)
123.123.123.123/000 ipv4    yes     no      no      YES (2)
1.2.3./0            ipv4    yes     no      no      YES (3)
1.2.3/0             ipv4    yes     no      no      YES -> agreed
1.2./0              ipv4    yes     no      no      YES (3)
1.2/0               ipv4    yes     no      no      YES -> agreed
1./0                ipv4    yes     no      no      YES (3)
1/0                 ipv4    yes     no      no      YES -> agreed

(1) this is by accident detected but should be not valid (e.g. 123.123.123.123/000 is not detected, see example below) (2) not a valid IPv4 address notation imho (3) do you really want to support that "strange" notation?

Also mask < 10 should be also only supported if having only one digit, e.g.

• /8 -> valid
• /08 -> invalid

[grayed]

                       Proposed        Release
Addrspec            detect? parse?  detect? parse?  change?
1.2.3.4             ipv4    yes     ipv4    yes     no
1.2.3.              ipv6rev yes     ipv6rev yes     no
1.2.3               ipv6rev yes     ipv6rev yes     no
1.2.                ipv6rev yes     ipv6rev yes     no
1.2                 ipv6rev no      ipv6rev no      no
1.                  ipv6rev no      ipv6rev no      no
1                   no      no      no      no      no
1.2.3.4/0           ipv4    yes     ipv4    yes     no
1.2.3.4/00          ipv4    yes     ipv4    yes     no
1.2.3.4/000         ipv4    yes     ipv4    yes     no (1)
1.2.3.4/00000000000 ipv4    yes     no      no      YES (2)
123.123.123.123/000 ipv4    yes     no      no      YES (2)
1.2.3./0            ipv4    yes     no      no      YES (3)
1.2.3/0             ipv4    yes     no      no      YES -> agreed
1.2./0              ipv4    yes     no      no      YES (3)
1.2/0               ipv4    yes     no      no      YES -> agreed
1./0                ipv4    yes     no      no      YES (3)
1/0                 ipv4    yes     no      no      YES -> agreed

(1) this is by accident detected but should be not valid (e.g. 123.123.123.123/000 is not detected, see example below) (2) not a valid IPv4 address notation imho

OK, done.

(3) do you really want to support that "strange" notation?

Well, I've seen it in the wild at some point. But since doesn't work in pfctl, I won't mind explicitly forbidding it either. :-)

Also mask < 10 should be also only supported if having only one digit, e.g.

• /8 -> valid
• /08 -> invalid

Yep, done as well.

[pbiering]

2 typos found, fixed in latest commit, please run always "make test" or "./autgen.sh" and watch output before submitting next patches, thank you very much!

[pbiering]

I've also improved the "ipv6rev" detection, as it was not really expected that something like "1.2." will be detected without having proper suffix ip6.arpa. Please test latest "master" whether it still works as you expect.