FS#17190 - [netcfg] 2.5rc1 auto wired ifplugd action script broken

Attached to Project: Arch Linux
Opened by Michael Kreitzer (mrgrim) - Wednesday, 18 November 2009, 14:38 GMT
Last edited by Andrea Scarpino (BaSh) - Saturday, 22 January 2011, 10:00 GMT
Task Type Bug Report
Category Packages: Core
Status Closed
Assigned To James Rayner (iphitus)
Thomas Bächler (brain0)
Architecture All
Severity High
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 3
Private No

Details

Description:

There were many issues with the ifplugd netcfg.action script that needed to be addressed before auto-wired worked at all:

First is this test on line 18:

[[ $CONNECTION != @(ethernet|ethernet-iproute) ]]

bash defaults to extglob disabled, and this construct is invalid without it. To corrected I added the following line before the case:

shopt -s extglob

Second, line 23:

for profile in $(list profiles}; do

I'm not sure how this happened. Looks like a pretty bad last minute fat finger w/o a test. Change to:

for profile in $(list_profiles); do

Third, line 26:

[[ $CONNECTION != "(ethernet|ethernet-iproute)" ]] && continue

This will never match. I'm certain a construct using @(..) identical to the one in the first loop was intended:

[[ $CONNECTION != @(ethernet|ethernet-iproute) ]] && continue

Fourth, the parenthesis around the two for loops cause bash to execute the loop body in a sub shell. This results in the exit commands exiting the sub shell and not the script proper. Ultimately this ends up in all profiles for an interface being attempted twice as both loops will complete (exit can be considered to act like a continue when the loop body is surrounded by parenthesis). This also causes the "exit 1" at the end of the script to always be reached causing ifplugd to think the action script always fails.

I'm attaching the full script after my modifications which works for me.

Thanks,
Michael
This task depends upon

Closed by  Andrea Scarpino (BaSh)
Saturday, 22 January 2011, 10:00 GMT
Reason for closing:  Fixed
Additional comments about closing:  http://projects.archlinux.org/netcfg.git /commit/?id=0fc35b1462b3257653ebca6d2df6 5a6062a2194a
Comment by Tobias Powalowski (tpowa) - Saturday, 28 November 2009, 17:28 GMT
isdn't this a dupclicate bug which is already fixed in git?
Comment by Maciej Sitarz (macieks2) - Saturday, 30 January 2010, 00:49 GMT
"Fourth, the parenthesis around the two for loops cause bash to execute the loop body in a sub shell. This results in the exit commands exiting the sub shell and not the script proper. Ultimately this ends up in all profiles for an interface being attempted twice as both loops will complete (exit can be considered to act like a continue when the loop body is surrounded by parenthesis). This also causes the "exit 1" at the end of the script to always be reached causing ifplugd to think the action script always fails."

The problem still exists in the git version of netcfg.action file:
http://projects.archlinux.org/netcfg.git/tree/ifplugd/netcfg.action

Simple patch:

17c17
< for profile in $(list_profiles); do
---
> for profile in $(list_profiles); do (
26,27c26,27
< done
< for profile in "${static_profiles[@]}"; do
---
> ) done
> for profile in "${static_profiles[@]}"; do (
30c30
< done
---
> ) done
Comment by Jim Pryor (Profjim) - Saturday, 30 January 2010, 00:58 GMT
Good catch. I think your patch is reversed, no? Anyway, here's a way to keep the subshell (and so isolate the load_profile) while still being able to bail:

for profile in $(list_profiles); do (
load_profile "$profile"
[[ "$INTERFACE" != "$1" ]] && continue
[[ "$CONNECTION" != @(ethernet|ethernet-iproute) ]] && continue
if [[ "$IP" == "dhcp" ]]; then
netcfg "$profile" && exit 1
else
static_profiles+=("$profile")
fi
) || exit 0; done

Comment by Maciej Sitarz (macieks2) - Saturday, 30 January 2010, 01:10 GMT
Yes, it's reversed, sorry. That's a good solution you provided.
Comment by James Rayner (iphitus) - Saturday, 30 January 2010, 23:44 GMT
looks like I'm releasing 2.5.1 shortly, I missed this one too. Will fix, sorry.
Comment by James Rayner (iphitus) - Sunday, 31 January 2010, 00:16 GMT
netcfg 2.5.1 has been upped to [testing]
Comment by Tobias Powalowski (tpowa) - Friday, 19 February 2010, 14:03 GMT
The actiond script is still not working with latest 2.5.2
the declare static_profiles is not working.
I don't understand the whole magic, but static_profiles is not reaching the second for loop.
It's empty, probably because the subshell exits and removes static_profile.
Comment by Tobias Powalowski (tpowa) - Friday, 19 February 2010, 14:06 GMT
here how it starts working for my small setup. I'm sure there is a much better way to fix the issue.
Comment by Tobias Powalowski (tpowa) - Friday, 19 February 2010, 14:47 GMT
next issue i found in autowired mode, the script checks profiles from network.d, but shouldn't it only check profiles activated in rc.conf?
Comment by Michael Kreitzer (mrgrim) - Friday, 19 February 2010, 15:29 GMT
My answer is not authoritative, but the way I understand the world the /etc/rc.d/network script manages static configurations defined in rc.conf. The netcfg tools are a separate method for interface configuration.
Comment by Tobias Powalowski (tpowa) - Friday, 19 February 2010, 15:50 GMT
Yes but i have 3 network profiles for eth0, and if auto-wired is activated it tries not the one specified in rc.conf. It tries all of them to activate.

Comment by James Rayner (iphitus) - Tuesday, 23 February 2010, 11:45 GMT
That's by design tpowa. The NETWORKS=() line defines the networks that rc.d/networks attempts to connect
Comment by Tobias Powalowski (tpowa) - Tuesday, 23 February 2010, 18:17 GMT
yes, that's what i think how it should work, but here it probes also those which are not in networks=()
Comment by Jim Pryor (Profjim) - Sunday, 28 February 2010, 13:04 GMT
The ifplugd/netcfg.action in 2.5.4 tries to append to static_profile inside a subshell. But those changes will exist only inside the subshell. I'm responsible for some (all?) of the broken code, sorry. Fix attached.
Comment by Ciriaco Garcia de Celis (cgarcia) - Saturday, 20 November 2010, 10:10 GMT
The current stable version is not functional. The last patch from Profjim works fine, but we still have a problem: if there are several static profiles for the interface a randon one is picked. For example I personally do have several profiles for eth0 (none is dhcp) and I need a particular one as the default, not just a random selection.

I agree that NETWORKS=() in rc.conf perhaps would not be currently the best place for selecting the net-auto-wired profile. In fact I tested such scenario by modifying the scripts, but the profile became loaded two times during boot: one from /etc/rc.d/net-profiles and a second from /etc/rc.d/net-auto-wired (ifplugd), this last one ending with an error in syslog because it was already loaded. A better solution it could have been a new AUTO_NETWORKS=() setting in rc.conf to select the auto _profile_, and using them to infere the interface(s) either wired or wireless (instead of the current WIRED_INTERFACE and WIRELESS_INTERFACE settings). But this would need a lot of changes, and would break the current way of doing the things.

In my case I finally have improved the Profjim solution to "flag" a particular profile as the "default" for auto start. If the profile makes true the AUTO_WIRED variable, such profile will be selected (even if there are dhcp ones). Thus the precedence is: AUTO_WIRED flagged profile -> any dhcp profile -> any static profile. If there are several AUTO_WIRED profiles for that network interface, the action script ends with error. Here I attach my netcfg.action customization, in case it helps.

Loading...