[pve-devel] [PATCH common] interfaces: support stanzas without types/methods

* [pve-devel] [PATCH common] interfaces: support stanzas without types/methods
@ 2024-04-23 13:03 Wolfgang Bumiller
  2024-04-23 14:32 ` [pve-devel] applied: " Thomas Lamprecht
  0 siblings, 1 reply; 2+ messages in thread
From: Wolfgang Bumiller @ 2024-04-23 13:03 UTC (permalink / raw)
  To: pve-devel

This is allowed in ifupdown2 and previously interfaces named
'vmbr\d+' were recognized as bridges even if they used this mode.
With commit e68ebda4f109 this is no longer the case.

Fixes: e68ebda4f109 ("fix #545: interfaces: allow arbitrary bridge names in network config")
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
The `__interface_to_string portion` is much better viewied with `-w`

 src/PVE/INotify.pm                            | 97 +++++++++++++------
 .../t.ifupdown2-typeless.pl                   | 47 +++++++++
 2 files changed, 117 insertions(+), 27 deletions(-)
 create mode 100644 test/etc_network_interfaces/t.ifupdown2-typeless.pl

diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
index 4568593..8a4a810 100644
--- a/src/PVE/INotify.pm
+++ b/src/PVE/INotify.pm
@@ -912,23 +912,29 @@ sub __read_etc_network_interfaces {
 	    # FIXME: handle those differently? auto makes it required on-boot, vs. best-effort
 	    $ifaces->{$_}->{autostart} = 1 for split (/\s+/, $2);
 
-	} elsif ($line =~ m/^\s*iface\s+(\S+)\s+(inet6?)\s+(\S+)\s*$/) {
+	} elsif ($line =~ m/^\s*iface\s+(\S+)(?:\s+(inet6?)\s+(\S+))?\s*$/) {
 	    my $i = $1;
 	    my $family = $2;
 	    my $f = { method => $3 }; # by family, merged to $d with a $suffix
-	    (my $suffix = $family) =~ s/^inet//;
+	    my $suffix = $family;
+	    $suffix =~ s/^inet// if defined $suffix;
 
 	    my $d = $ifaces->{$i} ||= {};
 	    $d->{priority} = $priority++ if !$d->{priority};
+
+	    # $family may be undef, an undef family means we have a stanza
+	    # without an `inet` or `inet6` section
 	    push @{$d->{families}}, $family;
 
+
 	    while (defined ($line = <$fh>)) {
 		$line =~ s/\s+$//; # drop trailing whitespaces
 
 		if ($line =~ m/^\s*#(.*?)\s*$/) {
-		    $f->{comments} = '' if !$f->{comments};
+		    my $pushto = defined($suffix) ? $f : $d;
+		    $pushto->{comments} = '' if !$pushto->{comments};
 		    my $comment = decode('UTF-8', $1);
-		    $f->{comments} .= "$comment\n";
+		    $pushto->{comments} .= "$comment\n";
 		} elsif ($line =~ m/^\s*(?:(?:iface|mapping|auto|source|source-directory)\s|allow-)/) {
 		    last;
 		} elsif ($line =~ m/^\s*((\S+)\s+(.+))$/) {
@@ -967,7 +973,17 @@ sub __read_etc_network_interfaces {
 		    };
 
 		    if ($id eq 'address' || $id eq 'netmask' || $id eq 'broadcast' || $id eq 'gateway') {
-			$f->{$id} = $value;
+			if (defined($suffix)) {
+			    $d->{$id.$suffix} = $value;
+			} elsif ($id ne 'netmask') {
+			    if ($value =~ /:/) {
+				$d->{$id.'6'} = $value;
+			    } else {
+				$d->{$id} = $value;
+			    }
+			} else {
+			    $d->{$id} = $value;
+			}
 		    } elsif ($simple_options->{$id}) {
 			$d->{$id} = $value;
 		    } elsif ($id eq 'slaves' || $id eq 'bridge_ports') {
@@ -1002,13 +1018,16 @@ sub __read_etc_network_interfaces {
 		    } elsif ($id eq 'vxlan-remoteip') {
 			push @{$d->{$id}}, $value;
 		    } else {
-			push @{$f->{options}}, $option;
+			my $pushto = defined($suffix) ? $f : $d;
+			push @{$pushto->{options}}, $option;
 		    }
 		} else {
 		    last;
 		}
 	    }
-	    $d->{"$_$suffix"} = $f->{$_} for keys $f->%*;
+	    if (defined($suffix)) {
+		$d->{"$_$suffix"} = $f->{$_} for keys $f->%*;
+	    }
 	    last SECTION if !defined($line);
 	    redo SECTION;
 	} elsif ($line =~ /\w/) {
@@ -1227,24 +1246,37 @@ sub _get_cidr {
 sub __interface_to_string {
     my ($iface, $d, $family, $first_block, $ifupdown2) = @_;
 
-    (my $suffix = $family) =~ s/^inet//;
+    my $suffix = $family;
+    $suffix =~ s/^inet// if defined($suffix);
 
-    return '' if !($d && $d->{"method$suffix"});
+    return '' if $family && !($d && $d->{"method$suffix"});
 
-    my $raw = "iface $iface $family " . $d->{"method$suffix"} . "\n";
+    my $raw = "iface $iface";
+    $raw .= " $family " . $d->{"method$suffix"} if defined $family;
+    $raw .= "\n";
 
-    if (my $addr = $d->{"address$suffix"}) {
-	if ($addr !~ /\/\d+$/ && $d->{"netmask$suffix"}) {
-	    if ($d->{"netmask$suffix"} =~ m/^\d+$/) {
-		$addr .= "/" . $d->{"netmask$suffix"};
-	    } elsif (my $mask = PVE::JSONSchema::get_netmask_bits($d->{"netmask$suffix"})) {
-		$addr .= "/" . $mask;
+    my $add_addr = sub {
+	my ($suffix) = @_;
+	if (my $addr = $d->{"address$suffix"}) {
+	    if ($addr !~ /\/\d+$/ && $d->{"netmask$suffix"}) {
+		if ($d->{"netmask$suffix"} =~ m/^\d+$/) {
+		    $addr .= "/" . $d->{"netmask$suffix"};
+		} elsif (my $mask = PVE::JSONSchema::get_netmask_bits($d->{"netmask$suffix"})) {
+		    $addr .= "/" . $mask;
+		}
 	    }
+	    $raw .= "\taddress ${addr}\n";
 	}
-	$raw .= "\taddress ${addr}\n";
-    }
 
-    $raw .= "\tgateway " . $d->{"gateway$suffix"} . "\n" if $d->{"gateway$suffix"};
+	$raw .= "\tgateway " . $d->{"gateway$suffix"} . "\n" if $d->{"gateway$suffix"};
+    };
+
+    if ($family) {
+	$add_addr->($suffix);
+    } else {
+	$add_addr->('');
+	$add_addr->('6');
+    }
 
     my $done = {
 	type => 1, priority => 1, method => 1, active => 1, exists => 1, comments => 1,
@@ -1413,14 +1445,25 @@ sub __interface_to_string {
 	}
     }
 
-    foreach my $option (@{$d->{"options$suffix"}}) {
-	$raw .= "\t$option\n";
-    }
+    my $add_options_comments = sub {
+	my ($suffix) = @_;
+
+	foreach my $option (@{$d->{"options$suffix"}}) {
+	    $raw .= "\t$option\n";
+	}
 
-    # add comments
-    my $comments = $d->{"comments$suffix"} || '';
-    foreach my $cl (split(/\n/, $comments)) {
-	$raw .= "#$cl\n";
+	# add comments
+	my $comments = $d->{"comments$suffix"} || '';
+	foreach my $cl (split(/\n/, $comments)) {
+	    $raw .= "#$cl\n";
+	}
+    };
+
+    if ($family) {
+	$add_options_comments->($suffix);
+    } else {
+	$add_options_comments->('');
+	$add_options_comments->('6');
     }
 
     $raw .= "\n";
@@ -1750,7 +1793,7 @@ NETWORKDOC
 	}
 
 	# if 'inet6' is the only family
-	if (scalar($d->{families}->@*) == 1 && $d->{families}[0] eq 'inet6') {
+	if (scalar($d->{families}->@*) == 1 && defined($d->{families}->[0]) && $d->{families}->[0] eq 'inet6') {
 	    $d->{comments6} = delete $d->{comments};
 	}
 
diff --git a/test/etc_network_interfaces/t.ifupdown2-typeless.pl b/test/etc_network_interfaces/t.ifupdown2-typeless.pl
new file mode 100644
index 0000000..d0ec5e6
--- /dev/null
+++ b/test/etc_network_interfaces/t.ifupdown2-typeless.pl
@@ -0,0 +1,47 @@
+my $ip = '10.0.0.2/24';
+my $gw = '10.0.0.1';
+my $ip6 = 'fc05::1:2/112';
+my $gw6 = 'fc05::1:1';
+
+r(load('base') . <<"EOF");
+auto vmbr1
+iface vmbr1
+	address 1.2.3.4/24
+	address fccc::a:1/64
+	gateway 1.2.3.1
+	gateway fccc::1
+	bridge-ports eth0
+	bridge-stp off
+	bridge-fd 0
+# Comment
+
+EOF
+
+my $run = 'first';
+my $ifaces = $config->{ifaces};
+
+my $ck = sub {
+    my ($i, $v, $e) = @_;
+    $ifaces->{$i}->{$v} eq $e
+	or die "$run run: $i variable $v: got \"$ifaces->{$i}->{$v}\", expected: $e\n";
+};
+
+my $check_config = sub {
+    $ck->('vmbr1', type => 'bridge');
+    $ck->('vmbr1', cidr => '1.2.3.4/24');
+    $ck->('vmbr1', gateway => '1.2.3.1');
+    $ck->('vmbr1', cidr6 => 'fccc::a:1/64');
+    $ck->('vmbr1', gateway6 => 'fccc::1');
+};
+
+$check_config->();
+
+# idempotency
+save('idem', w());
+r(load('idem'));
+expect load('idem');
+
+$run = 'second';
+$check_config->();
+
+1;
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 2+ messages in thread