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

Answer 1

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

Answer 2

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.