[netctl] Simplify init_profiles() implementation
diff mbox

Message ID 20180626102521.28129-1-emil.l.velikov@gmail.com
State New
Headers show

Commit Message

Eli Schwartz via arch-projects June 26, 2018, 10:25 a.m. UTC
From: Emil Velikov <emil.velikov@collabora.com>

Currently each profile is handles in two stages:
 - a unique value is returned for a set of patterns matches
 - depending on the value the profile/essid is added to global lists

A shorter and simpler solution is to omit the unnecessary value
passing/processing all together.

Cc: Jouke Witteveen <j.witteveen@gmail.com>
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
Jouke, let me know if you'd like this rebased on top of your patch.
Pardon if you're getting this twice. The initial submission was rejected
by the ML :-\
---
 src/wifi-menu | 40 ++++++++++++++--------------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

Comments

Eli Schwartz via arch-projects June 26, 2018, 10:40 a.m. UTC | #1
On Tue, Jun 26, 2018 at 12:27 PM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> From: Emil Velikov <emil.velikov@collabora.com>
>
> Currently each profile is handles in two stages:
>  - a unique value is returned for a set of patterns matches
>  - depending on the value the profile/essid is added to global lists
>
> A shorter and simpler solution is to omit the unnecessary value
> passing/processing all together.
>
> Cc: Jouke Witteveen <j.witteveen@gmail.com>
> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> ---
> Jouke, let me know if you'd like this rebased on top of your patch.
> Pardon if you're getting this twice. The initial submission was rejected
> by the ML :-\
> ---
>  src/wifi-menu | 40 ++++++++++++++--------------------------
>  1 file changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/src/wifi-menu b/src/wifi-menu
> index 42c4c53..928cf5e 100755
> --- a/src/wifi-menu
> +++ b/src/wifi-menu
> @@ -29,34 +29,22 @@ quote_safe() {
>      fi
>  }
>
> -# Fill PROFILES and ESSIDS with the profile names and essids of the profiles
> -# for interface $1
> +# Fill GENERATED, PROFILES and ESSIDS with the profile names and essids of the

Ah, right. I'll add "GENERATED".

> +# profiles for interface $1
>  init_profiles() {
> -    local i=0 essid profile
> +    local i=0 profile
>      while IFS= read -r profile; do
> -        essid=$(
> -            unset Interface ESSID
> -            source "$PROFILE_DIR/$profile" > /dev/null
> -            if [[ "$Interface" = "$1" && -n "$ESSID" ]]; then
> -                printf "%s" "$ESSID"
> -                if [[ "$Description" =~ "Automatically generated" ]]; then
> -                    return 2
> -                else
> -                    return 1
> -                fi
> -            fi
> -            return 0
> -        )
> -        case $? in
> -            2)
> -                GENERATED+=("$profile")
> -                ;&
> -            1)
> -                PROFILES[i]=$profile
> -                ESSIDS[i]=$essid
> -                (( ++i ))
> -                ;;
> -        esac

Okay, I'll add a comment explaining that we do things this way for
sandboxing reasons. We want to source the profile in a subshell to
contain any side-effects.

Thanks for your contribution!
- Jouke
Eli Schwartz via arch-projects June 26, 2018, 10:50 a.m. UTC | #2
On 26 June 2018 at 11:40, Jouke Witteveen <j.witteveen@gmail.com> wrote:
> On Tue, Jun 26, 2018 at 12:27 PM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>
>> From: Emil Velikov <emil.velikov@collabora.com>
>>
>> Currently each profile is handles in two stages:
>>  - a unique value is returned for a set of patterns matches
>>  - depending on the value the profile/essid is added to global lists
>>
>> A shorter and simpler solution is to omit the unnecessary value
>> passing/processing all together.
>>
>> Cc: Jouke Witteveen <j.witteveen@gmail.com>
>> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> ---
>> Jouke, let me know if you'd like this rebased on top of your patch.
>> Pardon if you're getting this twice. The initial submission was rejected
>> by the ML :-\
>> ---
>>  src/wifi-menu | 40 ++++++++++++++--------------------------
>>  1 file changed, 14 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/wifi-menu b/src/wifi-menu
>> index 42c4c53..928cf5e 100755
>> --- a/src/wifi-menu
>> +++ b/src/wifi-menu
>> @@ -29,34 +29,22 @@ quote_safe() {
>>      fi
>>  }
>>
>> -# Fill PROFILES and ESSIDS with the profile names and essids of the profiles
>> -# for interface $1
>> +# Fill GENERATED, PROFILES and ESSIDS with the profile names and essids of the
>
> Ah, right. I'll add "GENERATED".
>
>> +# profiles for interface $1
>>  init_profiles() {
>> -    local i=0 essid profile
>> +    local i=0 profile
>>      while IFS= read -r profile; do
>> -        essid=$(
>> -            unset Interface ESSID
>> -            source "$PROFILE_DIR/$profile" > /dev/null
>> -            if [[ "$Interface" = "$1" && -n "$ESSID" ]]; then
>> -                printf "%s" "$ESSID"
>> -                if [[ "$Description" =~ "Automatically generated" ]]; then
>> -                    return 2
>> -                else
>> -                    return 1
>> -                fi
>> -            fi
>> -            return 0
>> -        )
>> -        case $? in
>> -            2)
>> -                GENERATED+=("$profile")
>> -                ;&
>> -            1)
>> -                PROFILES[i]=$profile
>> -                ESSIDS[i]=$essid
>> -                (( ++i ))
>> -                ;;
>> -        esac
>
> Okay, I'll add a comment explaining that we do things this way for
> sandboxing reasons. We want to source the profile in a subshell to
> contain any side-effects.
>
Hmm, I did not think of that one. Thanks for the prompt explanation.

-Emil

Patch
diff mbox

diff --git a/src/wifi-menu b/src/wifi-menu
index 42c4c53..928cf5e 100755
--- a/src/wifi-menu
+++ b/src/wifi-menu
@@ -29,34 +29,22 @@  quote_safe() {
     fi
 }
 
-# Fill PROFILES and ESSIDS with the profile names and essids of the profiles
-# for interface $1
+# Fill GENERATED, PROFILES and ESSIDS with the profile names and essids of the
+# profiles for interface $1
 init_profiles() {
-    local i=0 essid profile
+    local i=0 profile
     while IFS= read -r profile; do
-        essid=$(
-            unset Interface ESSID
-            source "$PROFILE_DIR/$profile" > /dev/null
-            if [[ "$Interface" = "$1" && -n "$ESSID" ]]; then
-                printf "%s" "$ESSID"
-                if [[ "$Description" =~ "Automatically generated" ]]; then
-                    return 2
-                else
-                    return 1
-                fi
-            fi
-            return 0
-        )
-        case $? in
-            2)
-                GENERATED+=("$profile")
-                ;&
-            1)
-                PROFILES[i]=$profile
-                ESSIDS[i]=$essid
-                (( ++i ))
-                ;;
-        esac
+        unset Interface ESSID
+        source "$PROFILE_DIR/$profile" > /dev/null
+        if [[ "$Interface" != "$1" || -z "$ESSID" ]]; then
+            continue;
+        fi
+        if [[ "$Description" =~ "Automatically generated" ]]; then
+            GENERATED+=("$profile")
+        fi
+        PROFILES[i]=$profile
+        ESSIDS[i]=$ESSID
+        (( ++i ))
     done < <(list_profiles)
 }