Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

adding IPv6 option #20

Merged
merged 5 commits into from
May 11, 2017
Merged

adding IPv6 option #20

merged 5 commits into from
May 11, 2017

Conversation

mbruns
Copy link
Collaborator

@mbruns mbruns commented Mar 15, 2017

enables Netspoc to work on IPv6 topologies
IPv6 Testsuite can be generated via included makefile

@mbruns mbruns requested review from hknutzen and orrence March 15, 2017 08:09
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 99.96% when pulling eca4732 on adding_ipv6_option into b96c2a4 on master.

@mbruns
adding IPv6 option
f2c1b7e 
enables Netspoc to work on IPv6 topologies
IPv6 Testsuite can be generated via included makefile
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 99.96% when pulling 07afd5f on adding_ipv6_option into 24f540f on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 99.96% when pulling 0362d8c on adding_ipv6_option into 360eed0 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 99.96% when pulling 0362d8c on adding_ipv6_option into 360eed0 on master.

hknutzen
hknutzen reviewed Mar 15, 2017
View reviewed changes
Copy link
Owner

@hknutzen hknutzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sieht schon sehr gut aus. Habe aber einige Anmerkunen zu 10 Stellen.

bin/add-to-netspoc Outdated
@@ -272,6 +275,7 @@ pod2usage(-exitstatus => 0, -verbose => 2) if $man;

my $path = shift @ARGV or pod2usage(2);
$from_file or @ARGV or pod2usage(2);
$ip_pattern = $ipv6? "[a-f:\/0-9]+" : "[0-9.\/]+";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Können wir hier hier die Vereinigung der beiden Pattern verwenden, so dass der Code für beide Arten von IP-Adressen funktioniert?

t/ip-check.t
@@ -10,33 +10,6 @@ use Test_Netspoc;
my ($title, $in, $out);

############################################################
$title = 'Invalid IP addresses';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Der Test sollte nicht komplett gelöscht werden.

lib/Netspoc/Compiler/Common.pm Outdated
my $prefix = $config->{ipv6} == 1? 128 : 32;
while(1) {
last if $prefix == 0;
$prefix--;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bitweise ist nicht effizient. Dass kann besser in Happen zu 32 Bit erfolgen. Ich hatte dazu schon einmal eine Email geschickt.

lib/Netspoc/Compiler/Pass2.pm Outdated
sub prefix_code {
my ($ip_net) = @_;
my ($ip, $mask) = @{$ip_net}{qw(ip mask)};
my $ip_code = bitstr2ip($ip);
my $prefix_code = mask2prefix($mask);
return $prefix_code == 32 ? $ip_code : "$ip_code/$prefix_code";
return $prefix_code == ($config->{ipv6} == 1 ? 128 : 32)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sieht sehr unübersichtlich aus.

lib/Netspoc/Compiler/Pass1.pm Outdated

# 255.255.255.255, 127.255.255.255, ..., 0.0.0.3, 0.0.0.1, 0.0.0.0
my $bitstr_len = $config->{ipv6}? 128 : 32;
my @inverse_masks = map { ~ prefix2mask($_) } (0 .. $bitstr_len);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warum kann das keine globale Variable bleiben, die nur einmal initialiisiert wird? Jetzt wird das für jeden IP-Range erneut angelegt.

lib/Netspoc/Compiler/Pass1.pm Outdated

# A single bit, masking the lowest network bit.
my $next = $up_inv_mask & $mask;
my $next = $up_mask ^ $mask;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Der Kommentar passte zu dem vorherigen & (und). Aber das ^ (xor) müsste besser erklärt werden.

lib/Netspoc/Compiler/Pass1.pm Outdated
# $key is concatenation of two bit strings, split it into original
# bit strings. Bitstring length is 32 bit (4 bytes) for IPv4, and
# 128 bit (16 bytes) for IPv6.
my ($ip, $mask) = $config->{ipv6} == 1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Knackiger:
my $size = length($key) / 2;
unpack "a${size}a$size", $key;

lib/Netspoc/Compiler/Pass1.pm Outdated
ip => ip2bitstr($config->{ipv6} == 1 ?
'ff02::a' : '224.0.0.10'),
mask => ip2bitstr($config->{ipv6} == 1
?'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Das lange ffff:ffff... kommt mehrfach vor. Hier sollte noch eine geeignete Abstraktions-Schicht eingefügt werden.
Warum kann das gen_ip nicht für beide Fälle genutzt werden?
Denn zur Zeit ist gen_ip ungenutzt und deshalb die Test-Coverage schlechter geworden.

lib/Netspoc/Compiler/Pass1.pm Outdated
@@ -18185,6 +18246,12 @@ sub compile {

my ($in_path, $out_dir);
($config, $in_path, $out_dir) = get_args($args);
if ($config->{ipv6}) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Das muss weg.

lib/Netspoc/Compiler/Common.pm
our $zero_ip = pack('N', 0);
our $max_ip = pack('N', 0xffffffff);
our $zero_ip;
our $max_ip;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wir brauchen wohl auch noch eine globale Variable $prefix_len.
Denn das benutzen wir unten immer wieder.

hknutzen
hknutzen reviewed May 4, 2017
View reviewed changes
lib/Netspoc/Compiler/Common.pm Outdated
@@ -98,25 +100,88 @@ sub progress {

sub ip2bitstr {
my ($ip) = @_;
if ($config->{ipv6} == 1) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Der Test ($config->{ipv6} == 1) sollte besser als ($config->{ipv6}) geschrieben werden, denn uns interessiert nur der Wahrheitswert aber nicht die konkrete Zahl.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0001%) to 99.98% when pulling 7fa4232 on adding_ipv6_option into 360eed0 on master.

@mbruns mbruns merged commit 9cdc4b7 into master May 11, 2017
@hknutzen hknutzen deleted the adding_ipv6_option branch June 8, 2018 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hknutzen hknutzen hknutzen left review comments

@orrence orrence Awaiting requested review from orrence

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mbruns @coveralls @hknutzen