Also clear killed pinger and monitor pids

[moeller0]

https://github.com/lynxthecat/cake-autorate/blob/c80809586792291e9c5b49791a89804ae415d868/cake-autorate.sh#L637

    kill_and_wait_by_pid monitor_pids[$pinger] 0

does this actually work? I would expect

    kill_and_wait_by_pid ${monitor_pids[$pinger]} 0

(Sidenote, I really think all variables should be written with the surrounding {} as default as that helps to avoid bugs, so IMHO it is not ${variable} that needs a justifying comment, but an unadorned $variable, but I admit that is a matter of taste.)

after this we need to set

monitor_pids[$pinger]=

we should not keep known invalid PIDs around, if the process is killed we NEED to remove the pid from our book-keeping. And I think keeping an extra check around for empty. Same for kill_and_wait_by_pid ${pingerr_pids[$pinger]} 1 once killed remove the PID from the list... Well possible that the code already does that somewhere more clever and opaque, but IMHO it is quote helpful to keep tricky code to a minimum and open code stuff where performance is not of highest priority (and for all the killing performance does not matter all that much, as we do it rarely enough).

[lynxthecat]

cake-autorate/cake-autorate.sh

Line 637 in c808095

debug_cmd kill $pid
kill_and_wait_by_pid monitor_pids[$pinger] 0

does this actually work? I would expect

kill_and_wait_by_pid ${monitor_pids[$pinger]} 0

Yes because I want to pass both name of variable and value so inside the function use:

kill_and_wait_by_pid()
{
        local pid=${!1}
        local pid_name=$1

And this avoids having to pass name as extra argument.

(Sidenote, I really think all variables should be written with the surrounding {} as default as that helps to avoid bugs, so IMHO it is not ${variable} that needs a justifying comment, but an unadorned $variable, but I admit that is a matter of taste.)

Yeah perhaps all variables should be written that way but for now I'm just trying to keep everything consistent. At the moment I just use {} where there would otherwise be a problem with its absence e.g. like /var/run/${identifier}/

after this we need to set

monitor_pids[$pinger]= we should not keep known invalid PIDs around, if the process is killed we NEED to remove the pid from our book-keeping. And I think keeping an extra check around for empty. Same for kill_and_wait_by_pid ${pingerr_pids[$pinger]} 1 once killed remove the PID from the list... Well possible that the code already does that somewhere more clever and opaque, but IMHO it is quote helpful to keep tricky code to a minimum and open code stuff where performance is not of highest priority (and for all the killing performance does not matter all that much, as we do it rarely enough).

OK I agree - setting that now and will follow-up with commit in which this is fixed.

UPDATE: see this commit: See this commit: https://github.com/lynxthecat/cake-autorate/commit/63fff3a8fc398a8269dd8ec414a8dae59487a9f2

[moeller0]

kill_and_wait_by_pid()

If you switch this to kill by pid variable name I would at least change the function name ato not imply that it wants a numeric process ID as input.

I have zero problems with passing additional arguments in, after all we only call this from a few places, but that is as always your decision...

Yeah perhaps all variables should be written that way but for now I'm just trying to keep everything consistent. At the moment I just use {} where there would otherwise be a problem with its absence e.g. like /var/run/${identifier}/

Showing my ignorance in all matters shell, but:

root@turris:~# bash
root@turris:~# identifier=0 ls /var/run/${identifier}/
adblock.pid                             dnsmasq.cfg01411c.br-lan.dhcp           hostapd-wlan0.vlan                      pppoe-wan.pid                           suricata
atd.pid                                 foris-controller-client.sock            hostapd-wlan1.psk                       rainbow                                 turris-auth.trust
collectd                                fosquitto.pid                           hostapd-wlan1.vlan                      resolver.name                           ubus
config.md5                              fw3.lock                                libatsha204.lock                        resolver.pid                            watchdog.pid
cron.reboot                             fw3.state                               lighttpd.pid                            rpcd                                    wpa_supplicant
crond.pid                               hostapd                                 nmbd.pid                                samba                                   xtables.lock
ddns                                    hostapd-phy0.conf                       odhcp6c-duid.pppoe-wan                  smbd.pid
dnsmasq                                 hostapd-phy1.conf                       pakon-query.sock                        sqm
dnsmasq.cfg01411c.br-guest_turris.dhcp  hostapd-wlan0.psk                       pakon.sock                              sshd.pid
root@turris:~# identifier=0 ls /var/run/$identifier/
adblock.pid                             dnsmasq.cfg01411c.br-lan.dhcp           hostapd-wlan0.vlan                      pppoe-wan.pid                           suricata
atd.pid                                 foris-controller-client.sock            hostapd-wlan1.psk                       rainbow                                 turris-auth.trust
collectd                                fosquitto.pid                           hostapd-wlan1.vlan                      resolver.name                           ubus
config.md5                              fw3.lock                                libatsha204.lock                        resolver.pid                            watchdog.pid
cron.reboot                             fw3.state                               lighttpd.pid                            rpcd                                    wpa_supplicant
crond.pid                               hostapd                                 nmbd.pid                                samba                                   xtables.lock
ddns                                    hostapd-phy0.conf                       odhcp6c-duid.pppoe-wan                  smbd.pid
dnsmasq                                 hostapd-phy1.conf                       pakon-query.sock                        sqm
dnsmasq.cfg01411c.br-guest_turris.dhcp  hostapd-wlan0.psk                       pakon.sock                              sshd.pid
root@turris:~# 

this seems to be a case where the curly braces can be omitted. But again, I consider these to be integral parts of a shell variable and see little reason to save these two characters. I can understand not to change the over in bulk for an existing project, but anything I touch I would change over, but again, a matter of subjective preference and for cake-autorate your preference counts ;)

[lynxthecat]

Fixed here: https://github.com/lynxthecat/cake-autorate/commit/cd77880cfbbebad55863f8b95de01bffb92be53f.

Maybe we can mark this as closed now?