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

[Wolfgang Bumiller]

On Thu, May 12, 2016 at 02:30:11PM +0300, Igor Vlasenko wrote:
> 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.

This portion reads the interfaces from /proc/net/dev where the names are
terminated with a colon.