Closed kurahaupo closed 2 years ago
after a short review and some thinking i came to a conclusion that i dont want to become a project maintainer. initially i wanted to create a dead simple, extremely minimal and code correct cron without considering user experience and expecting users to never break anything. I was planning to get a lot of suggestions how to improve existing features and decrease the amount of code (that i am doing right now) and then archive it and make as a base for other more feature rich forks. i hope you understood me correctly, currently i want to just simplify and improve the existing code and then you are free to create a fork and make everything more advanced. you can or i will anyways look through changes you made in this pr to improve the existing code and preserve its extremely minimal state.
There are two specific situations which I think a script such as this should attempt to avoid:
These are not responsibilities that should be delegated to a secondary "debugger" script.
I get that you don't want something "complicated"; to have it easy to read and therefore reliable to maintain. However there are some parts that I do recommend, in approximate decreasing order of importance:
interval
; intr
is commonly short for "interrupt", which is confusing in the context of a time-sensitive system.SECONDS
variable that tracks the time since the shell started - or since you last set it.
In particular, if you set it to the current epoch-second, $SECONDS
will thereafter always track the epoch-second.
This removes a command from inside the loops.interval
and user
variables (and any others you add in future) before sourcing the config
file of each job, to avoid weirdness if the variable names are misspelled or missing.timer
files (rather than their containing directory); this gives you a free check that the file exists, and d=${t%/*}
is all that's necessary to get the containing directory.nullglob
setting to ensure that when there are no such files, you won't get complaints about a non-existent file with a *
in its name.config
until you have to. Once you've read the timer file, you know whether or not to run its job, and nothing in the config
will change that decision.
This avoids complaints about missing/unreadable/broken config
files for jobs that aren't ready to run anyway, so that you can freely edit a config
file any time as long as it's not going to run while you're editing it.timer
file as soon as you've decided to run its job, so that it's only re-created by writing the updated "next run time" into it.
This means that a broken job automatically gets disabled, without needing any specific handling (other than continue
or exit
).(( sleep_until >= timer || ( sleep_until = timer ) ))
Use a run_as
or SUDO
function, rather than setting a variable (this is one of the common malapropisms that ShellCheck will find). It's important to understand that function definitions are commands, and those statements need to be "executed" in order for the named functions to become defined. If you want the function to do different things depending on a configuration setting, you do not need to test the setting inside the function, but instead simply go
if [[ $setting = typeA ]]; then
function func { methodA; };
else
function func { methodB; };
fi
so that the definition of the function changes depending on the setting.
(Personal gripe: the way that doas
is installed by most package managers is, frankly, broken. It should be installed as an alternative version of sudo
, in the same way that postfix
, exim
, smail
, and msmtp
are installed as alternative versions of sendmail
, or that vim
, and nvi
are installed as alternative versions of vi
(and editor
). If you want to send email from a script, you always invoke /usr/sbin/sendmail
regardless of which MTA is installed; and if you want to run a job as another user, the packages ought to be installed so that you always invoke /usr/bin/sudo
regardless of whether that's actually the sudo
or doas
package.)
Whilst your script is indeed very simple, and that's a good thing, the problem is that it imports other code, and interacting code can be complex - and broken.
Unless the various code parts are thoroughly isolated from each other, error checking is pretty much the only way you will be able to prevent very weird things from happening. (Using a sub-shell would help to achieve that isolation, but would complicate the "sleep until" calculation.)
Having regression tests (your suggested "debugger" script) is great, but to get complete coverage you need to identify the points where values are used in the run-time code. You can either do that with tagged comments in the run-time code, or as actual test commands.
It's OK if you want to run in a mode with the error testing turned off, but I suggest the error-checking code should be left intact so that you can turn it on to diagnose problems.
Overall I would urge you to reconsider your position on error handling. My experience is that lack of error checking leads to weird bugs that are hard to find, and in the long run it's less work (and less pain) to centralize as much error handling as possible in one place rather than replicate it across many files.
same, excuse me i forgot to read this. i will apply some tweaks according to your suggestions
i am going to answer to each question number by order: 1(Hogging resources...) - a - it is not really cron specific because its up to the user what to run but i like your idea with exit status or broken cronjob configuration so it would be nice to see it in your fork (i am not going to implement that in minimal awecron) b - true, but again this is not a big issue and can be fixed with a miscellaneous cronjob that warns the user about the deletion of logs and then deletes them. (i am not going to implement that in minimal awecron) 2(Significantly skewing the times at which jobs are run...) - this can me fixed by running cronjobs in parallel that i have some experience with(if the goal is to get precise time it would also require creating time offset correction). I am not sure how though with the way minimal awecron works (i agree implementing this if the code wont massively change) 1(shellcheck) - i already did a lot and i am going to do that ones more at the end 2 - alright 3 - sure but i dont understand what specifically you are talking about 4 - good suggestion, i am going to test that 5 - i dont think that is required for minimal version, nothing bad happens (it also doesnt run the bin without $user) 6 - dont really understand, could you send a simple working example please 7 - oh yeah i had such issue, i will think about this 8 - "its expected that users never break anything" but if 6 is fine then this could be fine 9 - useful but requires rethinking of how the cron works (i will think about this at the end) 10 - so your suggestion is to sleep until some cronjob actually requires to wake up? this is a good idea (i will think about this at the end) 11 - fixed, su is more wide spread anyways and awecron supposed to run as root My experience is that lack of error checking leads to weird bugs that are hard to find... - i personally just set up ones and then chill without changing anything, it works sometimes also take a look at new code for issues.
On your recent update: rather than putting BASH_SOURCE
in the middle of the script, I would put Repo=${BASH_SOURCE[0]}
at the top of the file and use $Repo
at the top of the loop. (There will be cases where people don't want it to be $BASH_SOURCE
; having a separate settable variable makes it clearer how to fix it. The most likely case is where they put a symlink from wherever they installed it into /usr/sbin
, so that BASH_SOURCE
)
I've implemented almost all the previous suggestions in my version; sorry I didn't put comments in it to point them out.
Running jobs in parallel is possible, but it does indeed complicate things. And it increases the risk of CPU hogging.
Item (6) is already illustrated at lines 32-34 in my version:
for t in "$REPO"/*/timer; do
d=${t%/*}
c=$d/config
For item (8), just move the source
command from line 9 to line 14 (inside the if
/fi
block).
For item (10 - implement "precise sleep"), I combined the run and search into the main loop.
But an alternative approach would be to have a separate function to find which job will run next. You might find this more palatable:
Repo=${BASH_SOURCE[0]%/*}
MaxSleep=600 # wake up at least once every 10 minutes, to see if there are any new awecron jobs
MinSleep=0 # you can set this higher if you want
… your existing "main" function here …
function find_sleep_time {
local next_job_time
# Get the soonest time value from all of the timer files
read next_job_time < <(
# read all timer files, sort their contents in ascending numerical order
sort -n "$Repo"/*/timer
) || {
# "read" failed,
# which means the output from "sort" was empty,
# which means there were no valid awecron jobs.
delay=MaxSleep
return 0
}
# convert the "time to run" into "duration to sleep;
# then apply bounding limits to that, so it's
# neither shorter than MinSleep
# nor longer than MaxSleep
(( delay = next_job_time - SECONDS,
delay <= MaxSleep || ( delay = MaxSleep ),
delay >= MinSleep || ( delay = MinSleep ) ))
# return true/false (0/1) to indicate whether a sleep is actually necessary
(( delay > 0 ))
}
while true; do
main
find_sleep_time && sleep "$delay"
done
Note that for this to work, you need to ensure that the timer
file is a valid POSIX text file (every line ends with a linefeed, especially the last line, even moreso when it's the only line). That means updating line 19 from:
printf $(( time + intr )) > "$d/timer"
to:
printf '%d\n' $(( time + intr )) > "$d/timer"
or preferably (as per item 2):
printf '%d\n' $(( time + interval )) > "$d/timer"
(ShellCheck will complain about printf
lacking a format string; I'm making the additional point that the format string should end with \n
. This change won't have any effect on your existing line 11 timer=$(<"$d/timer")
because $(…)
is defined to discard trailing newlines.)
about BASH_SOURCE - its more about general way of location where files are saved. If actually make it usable and correctly installed on linux systems (it would also make security by default possible in permissions sense) then we should think of a place for cronjob directories, that could be /var/lib/awecron/ or /etc/awecron/ or /opt/awecron (i personally hate opt). But most likely minimal awecron would never be installed using package managers so cloning repo installation should be relatively fine, otherwise installation and update(maybe) scripts should be added. 6 - great! 8 - great! 10 - a great way to improve efficiency, i will look more into it while testing (most likely i am going to add it) setting time - ok i will fix that
after a short review and some thinking i came to a conclusion that i dont want to become a project maintainer. initially i wanted to create a dead simple, extremely minimal and code correct cron without considering user experience and expecting users to never break anything. I was planning to get a lot of suggestions how to improve existing features and decrease the amount of code (that i am doing right now) and then archive it and make as a base for other more feature rich forks. i hope you understood me correctly, currently i want to just simplify and improve the existing code and then you are free to create a fork and make everything more advanced. you can or i will anyways look through changes you made in this pr to improve the existing code and preserve its extremely minimal state.
Then have multiple people be maintainers.
ok it looks like you have made a lot of changes(thx). let me review them and try to understand