Having trouble with for loop to iterate over a directory

Question

I'm trying to run this part of my code in a magisk module on android so its using android mksh shell.

My goal is to skip the Fontchanger folder in $MODULESPATH since Fontchanger is my module, and if the folder is not fontchanger and it doesn't have a file named disable and it has a system/fonts directory inside $i, only then should the code run. However when I go install my magisk module zip, it runs this code detecting Fontchanger in $MODULESPATH and aborts. This is the opposite of what I need. It needs to skip the Fontchanger folder.

This is the code

    imageless_magisk && MODULESPATH=/data/adb/modules || MODULESPATH=/sbin/.core/img
    for i in $MODULESPATH/*; do
      if [ $i != Fontchanger ]; then
        if [ ! -f $i/disable ]; then
          if [ -d $i/system/fonts ]; then
            NAME=$(get_var $i/module.prop)
            ui_print " [!] "
            ui_print " [!] Module editing fonts detected [!] "
            ui_print " [!] Module - $NAME [!] "
            ui_print " [!] "
            abort
          fi
        fi
      fi
    done

    imageless_magisk && MODULESPATH=/data/adb/modules_update || MODULESPATH=/sbin/.core/img
    for i in $MODULESPATH/*; do
      if [ $i != Fontchanger ]; then
        if [ ! -f $i/disable ]; then
          if [ -d $i/system/fonts ]; then 
            NAME=$(get_var $i/module.prop)
            ui_print " [!] "
            ui_print " [!] Module editing fonts detected [!] "
            ui_print " [!] Module - $NAME [!] "
            ui_print " [!] "
            abort
          fi
        fi
      fi
    done


get_var() { sed -n 's/^name=//p' ${1}; }

imageless_magisk() {
  [ $MAGISK_VER_CODE -gt 18100 ]
  return $?
}

Thanks in advance for any and all help

I would add debugging statments like `echo "#dbg i = [$i]"` and `else` clauses `echo "#dbg :else found FontChanger"`. Good luck. — shellter, Sep 24 '19 at 22:27
so after commenting out set -euxo pipefail and set +euxo pipefail in my script the code now runs fine and doesnt detect the fontchanger folder like its supposed too. anyone have any ideas as to why removing set -euxo pipefail fixes it and allows it run as its supposed to? — john fawkes, Sep 25 '19 at 04:24

score 1 · Answer 1 · answered Sep 25 '19 at 13:20

mksh upstream developer here ☻

I normally advice people against set -e and especially set -u as they introduce failures in complex control structures that are not obvious. However, I don’t currently see anything using unused variables there. On the other hand, there’s an interaction between set -e and set -o pipefail described in the CAVEATS section of the latest edition of the upstream manual page (not yet released) which was only recently tracked down:

 Using set -o pipefail makes the following construct error out:

       set -e
       for x in 1 2; do
               false && echo $x
       done | cat

 This is because, while the “&&” ensures that the inner command's failure
 is not taken, it sets the entire for..done loop's errorlevel, which is
 passed on by -o pipefail.  Invert the inner command: true || echo $x

This may affect the nested ifs, but I’m not sure here.

On the other hand, this part of the code definitely checks the wrong thing:

if [ $i != Fontchanger ]; then

Remember, the line above that was…

for i in $MODULESPATH/*; do

… so $i would be something like $MODULESPATH/Fontchanger. Also, in the ! imageless_magisk case, you run over /sbin/.core/img twice.

Please allow me to propose a, hopefully equivalent, refactored code (I prefer a fail-first approach over nested ifs, and || conditions are more stable against set -e, and [[ conditionals are safer to use):

if imageless_magisk; then
        set -A MODULESPATHS -- /data/adb/modules /data/adb/modules_update
else
        set -A MODULESPATHS -- /sbin/.core/img
fi
for MODULESPATH in "${MODULESPATHS[@]}"; do
        for i in "$MODULESPATH"/*; do
                [[ $i = */Fontchanger ]] || continue
                [[ ! -f $i/disable ]] || continue
                [[ -d $i/system/fonts ]] || continue
                NAME=$(get_var "$i"/module.prop)
                ui_print " [!] "
                ui_print " [!] Module editing fonts detected [!] "
                ui_print " [!] Module - $NAME [!] "
                ui_print " [!] "
                abort
        done
done

You could even do something like this: fail only at the end, listing all directories that have “Module editing fonts”, instead of failing after the first occurrence:

if imageless_magisk; then
        set -A MODULESPATHS -- /data/adb/modules /data/adb/modules_update
else
        set -A MODULESPATHS -- /sbin/.core/img
fi
do_abort=0
for MODULESPATH in "${MODULESPATHS[@]}"; do
        for i in "$MODULESPATH"/*; do
                [[ $i = */Fontchanger ]] || continue
                [[ ! -f $i/disable ]] || continue
                [[ -d $i/system/fonts ]] || continue
                NAME=$(get_var "$i"/module.prop)
                if (( !do_abort )); then
                        ui_print " [!] "
                        ui_print " [!] Module editing fonts detected [!] "
                        do_abort=1
                fi
                ui_print " [!] Module - $NAME [!] "
        done
done
if (( do_abort )); then
        ui_print " [!] "
        abort
fi

These codes both should be set -u and set -eo pipefail safe. If they aren’t, I’d need a log (add set -x somewhere) to see where it breaks, as I don’t have a Magisk environment handy. The shell version (echo $KSH_VERSION) would also come handy; most Android ship with a really old mksh version.

Hope this helps; if not, feel free to ping me, and I’ll rework my answer.

So after much debugging and talking to a good friend of mine, we've come up with code that will work. @mirabilos it seems set - A doesn't work on Android as well. We'll it works in a terminal emulator but for some reason not in magisk — john fawkes, Sep 27 '19 at 23:27
`set -A arrayname` is a Korn Shell builtin and thus will work in any and all `mksh` versions. Do make sure your script is actually run with mksh — some third-party ROMs blindly replace `/system/bin/sh` with an inferiour (if alone for the fact it’s not adapted to Android) GNU bash or, worse, busybox sh, “because GNU/Linux does so”. A good thing to do is to debugging-print `${KSH_VERSION:-wrong shell}` near the beginning of the script. — mirabilos, Sep 28 '19 at 00:58
im using a stock rom and not custom. i think magisk is using bb sh instead of mksh but im not sure. im a magisk module repo moderator so i have close ties with topjohnwu so ill ask him to be sure. thanks again for your help! i really appreciate it — john fawkes, Sep 28 '19 at 21:20
just confirmed with topjohnwu magisk environment is using busybox sh. maybe i can find a way to override that — john fawkes, Sep 28 '19 at 22:20
Please ask the magisk people to use mksh instead, as it’s much more powerful and less buggy (and present on every Android from the never-released 2.4 onwards, 3.0, 4.0, 4.1, …). But good to have that confirmed. — mirabilos, Sep 29 '19 at 22:56
Topjohn said the reason he uses busybox sh is because of consistency. He packages busybox with magisk. I agree that mksh is much better than bb sh. Trying myself to break out of the bb sh during install and use mksh instead and I thought about using something like exec /system/bin/sh "$1" but the problem with this is everything in the script after that exec command doesn't run — john fawkes, Sep 30 '19 at 23:09
Without the `exec` ;) also, he can compile busybox without the sh applet, then it will use the system shell. — mirabilos, Oct 01 '19 at 00:19

score 0 · Accepted Answer · answered Sep 27 '19 at 23:28

MODULESPATH=/data/adb/modules
imageless_magisk || MODULESPATH=/sbin/.core/img

for i in $MODULESPATH*/*; do
  if [[ $i != *Fontchanger ]] && [ ! -f $i/disable ] && [ -d $i/system/fonts ]; then
    NAME=$(get_var $i/module.prop)
    ui_print " [!] "
    ui_print " [!] Module editing fonts detected [!] "
    ui_print " [!] Module - $NAME [!] "
    ui_print " [!] "
    cancel
  fi
done

This is the code we came up with that will work and is set - euxo pipefail compatible as well and works on Android in a magisk environment

Close enough, but still underquoted. – mirabilos Sep 28 '19 at 00:59 — mirabilos, Sep 28 '19 at 00:59

Having trouble with for loop to iterate over a directory

2 Answers2