Implement "system" crontabs, with documentation changes #30
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Retain the username on a file level; this is used for error reporting. This patch prepares for users in some contexts (eg. root in /etc/cron.d) to specify which user each job runs as.
Maintain the existing behaviour in other cases, where the username is specified from the file.
The username is ambiguous when displaying errors, as the root user can have multiple files. Reporting errors with the full context and filename should make it much easier to work out what is happening when something goes wrong. This also helps identify places where the username is actually used for function and not just logging.
No functional change here, just moving to its own function.
Start to attack the heavy indentation that is making it hard to understand the safety of the error handling.
No functional change; just refactor. Error conditions still handled the same.
A slight change in function when to stop parsing. The previous code would stop if all attributes were populated; we don't need this. Just always consume input until something is encountered that does not look like an attribute.
Channel all failures through the same function to deallocate line content. This ensures missing commands to not leak waiters etc.
We have "crontab" (the command) and "crontab" (the format description).
It needs further description of "system" crontabs which falls out of the scope of crontab.1. In general, it will benefit from being in a separate file where both pages can be broadened.
There's quite a lot of information here. Some of it, such as dependencies, is quite obscure and benefits from being separated out.
These can be moved to a separate section. Following man-pages(7) convention I chose "NOTES".
Mainly a re-ordering of the existing information, with the addition of a summary at the beginning. Man page convention forces the EXAMPLES to the end. Use this as an incentive to provide a better and more concise summary up-front.
Users do not need to know about this location; they should use the comman dto edit the crontab.
If the file starts with non-job content, such as a comment or newline, we return an invalid line which the caller then takes responsibilty for cleaning up.
Owner
|
Thank you for the contributions. I've been away from this codebase for a while, as you saw. I started diving back into it a few months ago but life intervened. I will definitely pick it up again, but it will take a month or two. |
Author
|
Thanks. The target here is Alpine Linux, I may see if they are interested in patching it downstream for now, as this seems to put it out of line with other distribution. I'm not keen to be maintaining forks of tools like cron for our infrastructure. It could serve as some testing as well. |
Owner
|
Great, Alpine is my distro of choice. I've been away from my Linux machine for a whole though.
…On Fri, Jul 24, 2020, at 11:45 AM, hills wrote:
Thanks. The target here is Alpine Linux, I may see if they are
interested in patching it downstream for now, as this seems to put it
out of line with other distribution. I'm not keen to be maintaining
forks of tools like cron for our infrastructure. It could serve as some
testing as well.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#30 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABYVXTL6MGTSAGGBCSZ553R5GUBZANCNFSM4PGXQXSQ>.
|
Author
|
Alpine won't be patching downstream; they are keeping to integration patches, not adding new features. So I hope we can review soon and keep upstream moving. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
dcron claims to support "system" crontabs, but differs in implementation to other crons by running jobs as root instead of allowing each job to have user specified.
Implement this behaviour, in the manner of anacron/vixie (RedHat) and FreeBSD.
Document the new behaviour.
Note that if the expectation is to be compatible with vendor-supplied cron.d scripts then more work may need to be done, as lines such as MAILTO= and PATH= are not supported by dcron.
On the way some necessary housekeeping was done:
Re-factored the file parsing into functions, so that error paths could be seen and reasoned more clearly. I needed to check this to insert error cases on the end.
Separated man page for "crontab" (command) and the file format and attempts to partition the page into subsections.
I kept the groundwork about as minimal as I could. Where patches are large, mostly they are moving existing code; the commit messages explain on a case-by-case basis.
I didn't verify the correctness or security of existing functionality such as how crond forks to run a user's task; just did my best to ensure it remained the same.
On the way I found the same compiler warning as everyone else; missing curly braces on an indentation block. I didn't want to stray into looking into this or verify its behavior, so I left it as is.