[pve-devel] [PATCH common] support for predictable network interface device names

[Igor Vlasenko]

On Thu, May 12, 2016 at 2:08 PM, Wolfgang Bumiller
<w.bumiller at proxmox.com> wrote:
> On Thu, May 12, 2016 at 11:42:29AM +0300, Igor Vlasenko wrote:
>> On Wed, May 11, 2016 at 10:56 PM, Igor Vlasenko <ivlasenko at gmail.com> wrote:
>> > This is an improved version of my previous patch
>> > [ support for udev-style physical interface names (like enp3s0),
>> >  http://pve.proxmox.com/pipermail/pve-devel/2016-May/020958.html ]
>> > thanks to Wolfgang.
>>
>> Yesterday I finished coding just before going to sleep, so I was a bit
>> muddleheaded and
>> left a few mistakes :( Here I fixed them and added a test case to verify.
>
> Ah now you added all the ones described in those links. But AFAIK wlan
> interfaces cannot be added to bridges.
> This seems a little overkill (and there are a few style concerns in the
> code).

I was thinking that repeating the same (possibly incomplete) pattern
again and again is the code clone bug pattern.
To have one regex in one place, though it looks a bit overkill, will help
to maintain the code in the future.

> Maybe it's best to stick to the devices we know users are currently
> dealing with and just stick to including 'en'-prefixed devices.
> Iow. a variant of your old patch with just 'en' instead of 'enp' and the
> veth/tap hunks removed.

I have no objections.

> Could you review the following modified version of your old patch?

seems good.
The only question I have does not concern the patch, but the current code
that is being patched.

--- a/src/PVE/INotify.pm
+++ b/src/PVE/INotify.pm
@@ -800,7 +800,7 @@ sub __read_etc_network_interfaces {

     if ($proc_net_dev) {
        while (defined ($line = <$proc_net_dev>)) {
-           if ($line =~ m/^\s*(eth\d+):.*/) {
+           if ($line =~ m/^\s*(eth\d+|en[^:.]+):.*/) {
                $ifaces->{$1}->{exists} = 1;
            }
        }

in the line 803:
-           if ($line =~ m/^\s*(eth\d+):.*/) {
Should it be a colon (`:') there? as in eth0:something?
Should not it be a point (`.') ? as in eth0.1?
If it is ok, then the patch is ok, else patch also should use a point
instead of the colon.


> ---
>  src/PVE/INotify.pm | 8 ++++----
>  src/PVE/Network.pm | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index 74a0fe1..c34659f 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -800,7 +800,7 @@ sub __read_etc_network_interfaces {
>
>      if ($proc_net_dev) {
>         while (defined ($line = <$proc_net_dev>)) {
> -           if ($line =~ m/^\s*(eth\d+):.*/) {
> +           if ($line =~ m/^\s*(eth\d+|en[^:.]+):.*/) {
>                 $ifaces->{$1}->{exists} = 1;
>             }
>         }
> @@ -973,7 +973,7 @@ sub __read_etc_network_interfaces {
>                 $ifaces->{$1}->{exists} = 0;
>                 $d->{exists} = 0;
>             }
> -       } elsif ($iface =~ m/^eth\d+$/) {
> +       } elsif ($iface =~ m/^(?:eth\d+|en[^:.]+)$/) {
>             if (!$d->{ovs_type}) {
>                 $d->{type} = 'eth';
>             } elsif ($d->{ovs_type} eq 'OVSPort') {
> @@ -1200,7 +1200,7 @@ sub __write_etc_network_interfaces {
>             $d->{type} eq 'OVSBond') {
>             my $brname = $used_ports->{$iface};
>             if (!$brname || !$ifaces->{$brname}) {
> -               if ($iface =~ /^eth/) {
> +               if ($iface =~ /^(?:eth|en)/) {
>                     $ifaces->{$iface} = { type => 'eth',
>                                           exists => 1,
>                                           method => 'manual',
> @@ -1289,7 +1289,7 @@ NETWORKDOC
>         my $pri;
>         if ($iface eq 'lo') {
>             $pri = $if_type_hash->{loopback};
> -       } elsif ($iface =~ m/^eth\d+$/) {
> +       } elsif ($iface =~ m/^(?:eth\d+|en[^:.]+)$/) {
>             $pri = $if_type_hash->{eth} + $child;
>         } elsif ($iface =~ m/^bond\d+$/) {
>             $pri = $if_type_hash->{bond} + $child;
> diff --git a/src/PVE/Network.pm b/src/PVE/Network.pm
> index be26132..0a984ad 100644
> --- a/src/PVE/Network.pm
> +++ b/src/PVE/Network.pm
> @@ -440,7 +440,7 @@ sub activate_bridge_vlan {
>
>      my @ifaces = ();
>      my $dir = "/sys/class/net/$bridge/brif";
> -    PVE::Tools::dir_glob_foreach($dir, '((eth|bond)\d+(\.\d+)?)', sub {
> +    PVE::Tools::dir_glob_foreach($dir, '(((eth|bond)\d+|en[^.]+)(\.\d+)?)', sub {
>          push @ifaces, $_[0];
>      });
>
> --
> 2.1.4
>
>



-- 
С уважением,
Игорь Власенко.