| Message ID | 20171221193502.31386-1-roman@advem.lv |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [LEDE-DEV] base-files: rc.common: fix enable() return code and logic | expand |
On 2017-12-21 21:35, Roman Yeryomin wrote: > In current state, if there is STOP but no START, enbale() > will return 0 (success), which is wrong. > Moreover there is no need to check for START/STOP twice. > Instead, add err variable to save success state and > and return it's value. > Also eliminate the need to disable() by using 'ln -sf', > which will first delete the old symlink if one exists. > Ah, mistake in description, if there is no STOP it returns 1, which is wrong. Will resend with corrected description. Regards, Roman
Roman Yeryomin <roman@advem.lv> wrote: > On 2017-12-21 21:35, Roman Yeryomin wrote: > > In current state, if there is STOP but no START, enbale() > > will return 0 (success), which is wrong. > > Moreover there is no need to check for START/STOP twice. > > Instead, add err variable to save success state and > > and return it's value. > > Also eliminate the need to disable() by using 'ln -sf', > > which will first delete the old symlink if one exists. > > > > Ah, mistake in description, if there is no STOP it returns 1, > which is wrong. Will resend with corrected description. What do you mean? Are you saying that "enable" doesn't work if STOP isn't defined? because that's clearly not the case... > > Regards, > Roman > > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev
On 2017-12-26 10:39, Karl Palsson wrote: > Roman Yeryomin <roman@advem.lv> wrote: >> On 2017-12-21 21:35, Roman Yeryomin wrote: >> > In current state, if there is STOP but no START, enbale() >> > will return 0 (success), which is wrong. >> > Moreover there is no need to check for START/STOP twice. >> > Instead, add err variable to save success state and >> > and return it's value. >> > Also eliminate the need to disable() by using 'ln -sf', >> > which will first delete the old symlink if one exists. >> > >> >> Ah, mistake in description, if there is no STOP it returns 1, >> which is wrong. Will resend with corrected description. > > What do you mean? Are you saying that "enable" doesn't work if > STOP isn't defined? because that's clearly not the case... > I didn't say it doesn't work, I said it is returning 1, which is error indication. Look at first check which returns 1 if there is no neither STOP nor START. It will pass if there is no STOP but afterwards second check will fail and enable() will return 1 anyway. It's pretty obvious that the logic is broken. If you implement error checking when using enable() then your check will be broken. Regards, Roman
diff --git a/package/base-files/files/etc/rc.common b/package/base-files/files/etc/rc.common index a2ea6a5679..a08c1f8f48 100755 --- a/package/base-files/files/etc/rc.common +++ b/package/base-files/files/etc/rc.common @@ -41,14 +41,15 @@ disable() { } enable() { + err=1 name="$(basename "${initscript}")" - disable - [ -n "$START" -o -n "$STOP" ] || { - echo "/etc/init.d/$name does not have a START or STOP value" - return 1 - } - [ "$START" ] && ln -s "../init.d/$name" "$IPKG_INSTROOT/etc/rc.d/S${START}${name##S[0-9][0-9]}" - [ "$STOP" ] && ln -s "../init.d/$name" "$IPKG_INSTROOT/etc/rc.d/K${STOP}${name##K[0-9][0-9]}" + [ "$START" ] && \ + ln -sf "../init.d/$name" "$IPKG_INSTROOT/etc/rc.d/S${START}${name##S[0-9][0-9]}" && \ + err=0 + [ "$STOP" ] && \ + ln -sf "../init.d/$name" "$IPKG_INSTROOT/etc/rc.d/K${STOP}${name##K[0-9][0-9]}" && \ + err=0 + return $err } enabled() {
In current state, if there is STOP but no START, enbale() will return 0 (success), which is wrong. Moreover there is no need to check for START/STOP twice. Instead, add err variable to save success state and and return it's value. Also eliminate the need to disable() by using 'ln -sf', which will first delete the old symlink if one exists. Signed-off-by: Roman Yeryomin <roman@advem.lv> --- package/base-files/files/etc/rc.common | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)