| Message ID | 20190722002425.6783-1-unixmania@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] package/dcron: fix startup error due to missing directories | expand |
Carlos, All, On 2019-07-21 21:24 -0300, unixmania@gmail.com spake thusly: > From: Carlos Santos <unixmania@gmail.com> > crond uses the /var/spool/cron/{crontabs,cronstamps} directories to > store the per-user crontabs and timestamps for jobs, respectively. > > On systems with busybox or sysv init /var/spool is by default a link to > /tmp (see package/skeleton-init-sysv/skeleton/). So if those directories > are created at installation time they actually are under /tmp/spool and > vanish at run time when a tmpfs is mounted at /tmp. In this case crond > issues error messages that can't be seen. > > The error is hidden because we use start-stop-daemon to run crond in > foreground mode to create a pid file and move crond to background. In > this mode crond does not use syslog and all error messages are lost > because start-stop-daemon redirects strderr to /dev/null. That is a bit unfortunate, indeed... :-( > Getting a presistent storage for /var/spool would require customisations > of the directory layout, which can not be done in a generic way, as each > one will need to adapt to their own constraints. > > Fix these problem by means of the following changes: > > - Add configs for the crontabs and cronstamps paths. They can be used to > move the directories to persistent filesystems, if necessary. > - Change the init script and systemd unit to ensure that the directories > are created before starting crond. > - Start crond in daemon mode in the init script and use "pidof crond" to > create the required pid file. I think there are at least two issues at stake, and that each issue should be fixed with its own patch. The first patch should fix the syslog issue. The second patch whould address the directory locations. However, I am still not sure we want to allow users to provide such options. Otherwise, we'd have to offer that option for all packages that want to store a state. There are a lot of such packages. Instead, my position is still that we should not allow this, and use the defaults for the packages. Alternatively, people can provide their /etc/default/$DEAMON config file that overrides the default locations. Additionally, if a service needs a directory to exist at runtime, they shall ensure in their init script that the directory exist. See for example the dhcp package that does exactly what you want to do for dcron: $ cat package/dhcp/S80dhcp-server ... [ -r "${CFG_FILE}" ] && . "${CFG_FILE}" ... case "$1" in start) printf "Starting DHCP server: " test -d /var/lib/dhcp/ || mkdir -p /var/lib/dhcp/ test -f /var/lib/dhcp/dhcpd.leases || touch /var/lib/dhcp/dhcpd.leases start-stop-daemon -S -q -x ${DAEMON} -- -q $OPTIONS $INTERFACES ... Regards, Yann E. MORIN. > Fixes: https://bugs.busybox.net/show_bug.cgi?id=12011 > > Signed-off-by: Carlos Santos <unixmania@gmail.com> > --- > Changes v1->v2: > - Remove (partially), since the fix is now complete > --- > package/dcron/Config.in | 16 ++++++++++++++++ > package/dcron/S90dcron | 8 +++++++- > package/dcron/dcron.mk | 11 +++++++++-- > package/dcron/dcron.service | 3 ++- > 4 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/package/dcron/Config.in b/package/dcron/Config.in > index d7f66bdb7d..1203c42dec 100644 > --- a/package/dcron/Config.in > +++ b/package/dcron/Config.in > @@ -21,3 +21,19 @@ config BR2_PACKAGE_DCRON > with sgid bit enabled. > > http://www.jimpryor.net/linux/dcron.html > + > +if BR2_PACKAGE_DCRON > + > +config BR2_PACKAGE_DCRON_CRONTABS_DIR > + string "crontabs directory" > + default "/var/spool/cron/crontabs" > + help > + Directory of per-user crontabs > + > +config BR2_PACKAGE_DCRON_CRONSTAMPS_DIR > + string "cronstamps directory" > + default "/var/spool/cron/cronstamps" > + help > + Directory of timestamps for @freq and FREQ=... jobs > + > +endif > diff --git a/package/dcron/S90dcron b/package/dcron/S90dcron > index de21d2ca13..4527277623 100644 > --- a/package/dcron/S90dcron > +++ b/package/dcron/S90dcron > @@ -1,9 +1,15 @@ > #!/bin/sh > > +CRONTABS_DIR=%%CRONTABS_DIR%% > +CRONSTAMPS_DIR=%%CRONSTAMPS_DIR%% > + > case "$1" in > start) > printf "Starting cron ... " > - start-stop-daemon -S -q -m -b -p /var/run/dcron.pid --exec /usr/sbin/crond -- -f > + mkdir -p "$CRONTABS_DIR" "$CRONSTAMPS_DIR" > + start-stop-daemon -S -q -p /var/run/dcron.pid -x /usr/sbin/crond \ > + -- -c "$CRONTABS_DIR" -t "$CRONSTAMPS_DIR" > + pidof crond > /var/run/dcron.pid || rm -f /var/run/dcron.pid > echo "done." > ;; > stop) > diff --git a/package/dcron/dcron.mk b/package/dcron/dcron.mk > index 2ee0709af5..453e3acb54 100644 > --- a/package/dcron/dcron.mk > +++ b/package/dcron/dcron.mk > @@ -19,18 +19,25 @@ define DCRON_INSTALL_TARGET_CMDS > $(INSTALL) -D -m0644 $(@D)/extra/root.crontab $(TARGET_DIR)/etc/cron.d/system > # Busybox provides run-parts, so there is no need to use nor install provided run-cron > $(SED) 's#/usr/sbin/run-cron#/bin/run-parts#g' $(TARGET_DIR)/etc/cron.d/system > - $(INSTALL) -d -m0755 $(TARGET_DIR)/var/spool/cron/crontabs \ > - $(TARGET_DIR)/etc/cron.daily $(TARGET_DIR)/etc/cron.hourly \ > + $(INSTALL) -d -m750 $(TARGET_DIR)/etc/cron.daily $(TARGET_DIR)/etc/cron.hourly \ > $(TARGET_DIR)/etc/cron.monthly $(TARGET_DIR)/etc/cron.weekly > endef > > define DCRON_INSTALL_INIT_SYSV > $(INSTALL) -D -m 0755 package/dcron/S90dcron $(TARGET_DIR)/etc/init.d/S90dcron > + $(SED) 's#%%CRONTABS_DIR%%#$(BR2_PACKAGE_DCRON_CRONTABS_DIR)#' \ > + $(TARGET_DIR)/etc/init.d/S90dcron > + $(SED) 's#%%CRONSTAMPS_DIR%%#$(BR2_PACKAGE_DCRON_CRONSTAMPS_DIR)#' \ > + $(TARGET_DIR)/etc/init.d/S90dcron > endef > > define DCRON_INSTALL_INIT_SYSTEMD > $(INSTALL) -D -m 644 package/dcron/dcron.service \ > $(TARGET_DIR)/usr/lib/systemd/system/dcron.service > + $(SED) 's#%%CRONTABS_DIR%%#$(BR2_PACKAGE_DCRON_CRONTABS_DIR)#' \ > + $(TARGET_DIR)/usr/lib/systemd/system/dcron.service > + $(SED) 's#%%CRONSTAMPS_DIR%%#$(BR2_PACKAGE_DCRON_CRONSTAMPS_DIR)#' \ > + $(TARGET_DIR)/usr/lib/systemd/system/dcron.service > mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants > ln -sf ../../../../usr/lib/systemd/system/dcron.service \ > $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/dcron.service > diff --git a/package/dcron/dcron.service b/package/dcron/dcron.service > index 924ed72205..abce25ba4f 100644 > --- a/package/dcron/dcron.service > +++ b/package/dcron/dcron.service > @@ -3,7 +3,8 @@ Description=Task scheduler daemon > After=syslog.target > > [Service] > -ExecStart=/usr/sbin/crond -S > +ExecStartPre=mkdir -p %%CRONTABS_DIR%% %%CRONSTAMPS_DIR%% > +ExecStart=/usr/sbin/crond -c %%CRONTABS_DIR%% -t %%CRONSTAMPS_DIR%% > Type=forking > > [Install] > -- > 2.18.1 >
Carlos, All, On 2019-07-21 21:24 -0300, unixmania@gmail.com spake thusly: > From: Carlos Santos <unixmania@gmail.com> > crond uses the /var/spool/cron/{crontabs,cronstamps} directories to > store the per-user crontabs and timestamps for jobs, respectively. > > On systems with busybox or sysv init /var/spool is by default a link to > /tmp (see package/skeleton-init-sysv/skeleton/). So if those directories > are created at installation time they actually are under /tmp/spool and > vanish at run time when a tmpfs is mounted at /tmp. In this case crond > issues error messages that can't be seen. > > The error is hidden because we use start-stop-daemon to run crond in > foreground mode to create a pid file and move crond to background. In > this mode crond does not use syslog and all error messages are lost > because start-stop-daemon redirects strderr to /dev/null. > > Getting a presistent storage for /var/spool would require customisations > of the directory layout, which can not be done in a generic way, as each > one will need to adapt to their own constraints. > > Fix these problem by means of the following changes: > > - Add configs for the crontabs and cronstamps paths. They can be used to > move the directories to persistent filesystems, if necessary. > - Change the init script and systemd unit to ensure that the directories > are created before starting crond. > - Start crond in daemon mode in the init script and use "pidof crond" to > create the required pid file. > > Fixes: https://bugs.busybox.net/show_bug.cgi?id=12011 I did provide some review on this patch, but I haven't seen any reply or repost. I've marked it changes-requested in Ptchwork. Feel free to respin a fixed up version that addresses my comments (either in the commit log, or as different changes). Thanks! Regards, Yann E. MORIN. > Signed-off-by: Carlos Santos <unixmania@gmail.com> > --- > Changes v1->v2: > - Remove (partially), since the fix is now complete > --- > package/dcron/Config.in | 16 ++++++++++++++++ > package/dcron/S90dcron | 8 +++++++- > package/dcron/dcron.mk | 11 +++++++++-- > package/dcron/dcron.service | 3 ++- > 4 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/package/dcron/Config.in b/package/dcron/Config.in > index d7f66bdb7d..1203c42dec 100644 > --- a/package/dcron/Config.in > +++ b/package/dcron/Config.in > @@ -21,3 +21,19 @@ config BR2_PACKAGE_DCRON > with sgid bit enabled. > > http://www.jimpryor.net/linux/dcron.html > + > +if BR2_PACKAGE_DCRON > + > +config BR2_PACKAGE_DCRON_CRONTABS_DIR > + string "crontabs directory" > + default "/var/spool/cron/crontabs" > + help > + Directory of per-user crontabs > + > +config BR2_PACKAGE_DCRON_CRONSTAMPS_DIR > + string "cronstamps directory" > + default "/var/spool/cron/cronstamps" > + help > + Directory of timestamps for @freq and FREQ=... jobs > + > +endif > diff --git a/package/dcron/S90dcron b/package/dcron/S90dcron > index de21d2ca13..4527277623 100644 > --- a/package/dcron/S90dcron > +++ b/package/dcron/S90dcron > @@ -1,9 +1,15 @@ > #!/bin/sh > > +CRONTABS_DIR=%%CRONTABS_DIR%% > +CRONSTAMPS_DIR=%%CRONSTAMPS_DIR%% > + > case "$1" in > start) > printf "Starting cron ... " > - start-stop-daemon -S -q -m -b -p /var/run/dcron.pid --exec /usr/sbin/crond -- -f > + mkdir -p "$CRONTABS_DIR" "$CRONSTAMPS_DIR" > + start-stop-daemon -S -q -p /var/run/dcron.pid -x /usr/sbin/crond \ > + -- -c "$CRONTABS_DIR" -t "$CRONSTAMPS_DIR" > + pidof crond > /var/run/dcron.pid || rm -f /var/run/dcron.pid > echo "done." > ;; > stop) > diff --git a/package/dcron/dcron.mk b/package/dcron/dcron.mk > index 2ee0709af5..453e3acb54 100644 > --- a/package/dcron/dcron.mk > +++ b/package/dcron/dcron.mk > @@ -19,18 +19,25 @@ define DCRON_INSTALL_TARGET_CMDS > $(INSTALL) -D -m0644 $(@D)/extra/root.crontab $(TARGET_DIR)/etc/cron.d/system > # Busybox provides run-parts, so there is no need to use nor install provided run-cron > $(SED) 's#/usr/sbin/run-cron#/bin/run-parts#g' $(TARGET_DIR)/etc/cron.d/system > - $(INSTALL) -d -m0755 $(TARGET_DIR)/var/spool/cron/crontabs \ > - $(TARGET_DIR)/etc/cron.daily $(TARGET_DIR)/etc/cron.hourly \ > + $(INSTALL) -d -m750 $(TARGET_DIR)/etc/cron.daily $(TARGET_DIR)/etc/cron.hourly \ > $(TARGET_DIR)/etc/cron.monthly $(TARGET_DIR)/etc/cron.weekly > endef > > define DCRON_INSTALL_INIT_SYSV > $(INSTALL) -D -m 0755 package/dcron/S90dcron $(TARGET_DIR)/etc/init.d/S90dcron > + $(SED) 's#%%CRONTABS_DIR%%#$(BR2_PACKAGE_DCRON_CRONTABS_DIR)#' \ > + $(TARGET_DIR)/etc/init.d/S90dcron > + $(SED) 's#%%CRONSTAMPS_DIR%%#$(BR2_PACKAGE_DCRON_CRONSTAMPS_DIR)#' \ > + $(TARGET_DIR)/etc/init.d/S90dcron > endef > > define DCRON_INSTALL_INIT_SYSTEMD > $(INSTALL) -D -m 644 package/dcron/dcron.service \ > $(TARGET_DIR)/usr/lib/systemd/system/dcron.service > + $(SED) 's#%%CRONTABS_DIR%%#$(BR2_PACKAGE_DCRON_CRONTABS_DIR)#' \ > + $(TARGET_DIR)/usr/lib/systemd/system/dcron.service > + $(SED) 's#%%CRONSTAMPS_DIR%%#$(BR2_PACKAGE_DCRON_CRONSTAMPS_DIR)#' \ > + $(TARGET_DIR)/usr/lib/systemd/system/dcron.service > mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants > ln -sf ../../../../usr/lib/systemd/system/dcron.service \ > $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/dcron.service > diff --git a/package/dcron/dcron.service b/package/dcron/dcron.service > index 924ed72205..abce25ba4f 100644 > --- a/package/dcron/dcron.service > +++ b/package/dcron/dcron.service > @@ -3,7 +3,8 @@ Description=Task scheduler daemon > After=syslog.target > > [Service] > -ExecStart=/usr/sbin/crond -S > +ExecStartPre=mkdir -p %%CRONTABS_DIR%% %%CRONSTAMPS_DIR%% > +ExecStart=/usr/sbin/crond -c %%CRONTABS_DIR%% -t %%CRONSTAMPS_DIR%% > Type=forking > > [Install] > -- > 2.18.1 >
diff --git a/package/dcron/Config.in b/package/dcron/Config.in index d7f66bdb7d..1203c42dec 100644 --- a/package/dcron/Config.in +++ b/package/dcron/Config.in @@ -21,3 +21,19 @@ config BR2_PACKAGE_DCRON with sgid bit enabled. http://www.jimpryor.net/linux/dcron.html + +if BR2_PACKAGE_DCRON + +config BR2_PACKAGE_DCRON_CRONTABS_DIR + string "crontabs directory" + default "/var/spool/cron/crontabs" + help + Directory of per-user crontabs + +config BR2_PACKAGE_DCRON_CRONSTAMPS_DIR + string "cronstamps directory" + default "/var/spool/cron/cronstamps" + help + Directory of timestamps for @freq and FREQ=... jobs + +endif diff --git a/package/dcron/S90dcron b/package/dcron/S90dcron index de21d2ca13..4527277623 100644 --- a/package/dcron/S90dcron +++ b/package/dcron/S90dcron @@ -1,9 +1,15 @@ #!/bin/sh +CRONTABS_DIR=%%CRONTABS_DIR%% +CRONSTAMPS_DIR=%%CRONSTAMPS_DIR%% + case "$1" in start) printf "Starting cron ... " - start-stop-daemon -S -q -m -b -p /var/run/dcron.pid --exec /usr/sbin/crond -- -f + mkdir -p "$CRONTABS_DIR" "$CRONSTAMPS_DIR" + start-stop-daemon -S -q -p /var/run/dcron.pid -x /usr/sbin/crond \ + -- -c "$CRONTABS_DIR" -t "$CRONSTAMPS_DIR" + pidof crond > /var/run/dcron.pid || rm -f /var/run/dcron.pid echo "done." ;; stop) diff --git a/package/dcron/dcron.mk b/package/dcron/dcron.mk index 2ee0709af5..453e3acb54 100644 --- a/package/dcron/dcron.mk +++ b/package/dcron/dcron.mk @@ -19,18 +19,25 @@ define DCRON_INSTALL_TARGET_CMDS $(INSTALL) -D -m0644 $(@D)/extra/root.crontab $(TARGET_DIR)/etc/cron.d/system # Busybox provides run-parts, so there is no need to use nor install provided run-cron $(SED) 's#/usr/sbin/run-cron#/bin/run-parts#g' $(TARGET_DIR)/etc/cron.d/system - $(INSTALL) -d -m0755 $(TARGET_DIR)/var/spool/cron/crontabs \ - $(TARGET_DIR)/etc/cron.daily $(TARGET_DIR)/etc/cron.hourly \ + $(INSTALL) -d -m750 $(TARGET_DIR)/etc/cron.daily $(TARGET_DIR)/etc/cron.hourly \ $(TARGET_DIR)/etc/cron.monthly $(TARGET_DIR)/etc/cron.weekly endef define DCRON_INSTALL_INIT_SYSV $(INSTALL) -D -m 0755 package/dcron/S90dcron $(TARGET_DIR)/etc/init.d/S90dcron + $(SED) 's#%%CRONTABS_DIR%%#$(BR2_PACKAGE_DCRON_CRONTABS_DIR)#' \ + $(TARGET_DIR)/etc/init.d/S90dcron + $(SED) 's#%%CRONSTAMPS_DIR%%#$(BR2_PACKAGE_DCRON_CRONSTAMPS_DIR)#' \ + $(TARGET_DIR)/etc/init.d/S90dcron endef define DCRON_INSTALL_INIT_SYSTEMD $(INSTALL) -D -m 644 package/dcron/dcron.service \ $(TARGET_DIR)/usr/lib/systemd/system/dcron.service + $(SED) 's#%%CRONTABS_DIR%%#$(BR2_PACKAGE_DCRON_CRONTABS_DIR)#' \ + $(TARGET_DIR)/usr/lib/systemd/system/dcron.service + $(SED) 's#%%CRONSTAMPS_DIR%%#$(BR2_PACKAGE_DCRON_CRONSTAMPS_DIR)#' \ + $(TARGET_DIR)/usr/lib/systemd/system/dcron.service mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants ln -sf ../../../../usr/lib/systemd/system/dcron.service \ $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/dcron.service diff --git a/package/dcron/dcron.service b/package/dcron/dcron.service index 924ed72205..abce25ba4f 100644 --- a/package/dcron/dcron.service +++ b/package/dcron/dcron.service @@ -3,7 +3,8 @@ Description=Task scheduler daemon After=syslog.target [Service] -ExecStart=/usr/sbin/crond -S +ExecStartPre=mkdir -p %%CRONTABS_DIR%% %%CRONSTAMPS_DIR%% +ExecStart=/usr/sbin/crond -c %%CRONTABS_DIR%% -t %%CRONSTAMPS_DIR%% Type=forking [Install]