| Message ID | 20180413162521.7078-2-casantos@datacom.ind.br |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix/improve modem-manager init script | expand |
Hello, On Fri, 13 Apr 2018 13:25:20 -0300, Carlos Santos wrote: > MODEMMANAGER_BIN was never set. Fortunately it did not break the script > execution due to the permissive behavior of "test": > > $ test -x non-existing || echo error > error > $ test -x || echo error > test -x && echo success > success > > Signed-off-by: Carlos Santos <casantos@datacom.ind.br> > --- > package/modem-manager/S44modem-manager | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/package/modem-manager/S44modem-manager b/package/modem-manager/S44modem-manager > index cf6d89601d..05803fe198 100755 > --- a/package/modem-manager/S44modem-manager > +++ b/package/modem-manager/S44modem-manager > @@ -3,6 +3,7 @@ > # Starts ModemManager > # > > +MODEMMANAGER_BIN=/usr/sbin/ModemManager > PIDFILE=/var/run/ModemManager.pid > > [ -x $MODEMMANAGER_BIN ] || exit 0 In fact, I think this test doesn't make sense. It just exits without any error if the binary doesn't exists, which is silly. I'm advocating for removing such tests altogether, which would make the MODEMMANAGER_BIN variable unnecessary. Thomas
> From: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com> > To: "Carlos Santos" <casantos@datacom.ind.br> > Cc: "buildroot" <buildroot@buildroot.org>, "Aleksander Morgado" <aleksander@aleksander.es> > Sent: Saturday, April 14, 2018 1:07:25 PM > Subject: Re: [Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script > Hello, > > On Fri, 13 Apr 2018 13:25:20 -0300, Carlos Santos wrote: >> MODEMMANAGER_BIN was never set. Fortunately it did not break the script >> execution due to the permissive behavior of "test": >> >> $ test -x non-existing || echo error >> error >> $ test -x || echo error >> test -x && echo success >> success >> >> Signed-off-by: Carlos Santos <casantos@datacom.ind.br> >> --- >> package/modem-manager/S44modem-manager | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/package/modem-manager/S44modem-manager >> b/package/modem-manager/S44modem-manager >> index cf6d89601d..05803fe198 100755 >> --- a/package/modem-manager/S44modem-manager >> +++ b/package/modem-manager/S44modem-manager >> @@ -3,6 +3,7 @@ >> # Starts ModemManager >> # >> >> +MODEMMANAGER_BIN=/usr/sbin/ModemManager >> PIDFILE=/var/run/ModemManager.pid >> >> [ -x $MODEMMANAGER_BIN ] || exit 0 > > In fact, I think this test doesn't make sense. It just exits without > any error if the binary doesn't exists, which is silly. > > I'm advocating for removing such tests altogether, which would make the > MODEMMANAGER_BIN variable unnecessary. I think the original purpose of these test was allowing the user to disable the service by chmod'ing -x the executable file. We discussed a similar situation in https://patchwork.ozlabs.org/patch/818897/ [which by the way is still in my endless to-do list]
Hello, On Sun, 15 Apr 2018 12:31:49 -0300 (BRT), Carlos Santos wrote: > >> +MODEMMANAGER_BIN=/usr/sbin/ModemManager > >> PIDFILE=/var/run/ModemManager.pid > >> > >> [ -x $MODEMMANAGER_BIN ] || exit 0 > > > > In fact, I think this test doesn't make sense. It just exits without > > any error if the binary doesn't exists, which is silly. > > > > I'm advocating for removing such tests altogether, which would make the > > MODEMMANAGER_BIN variable unnecessary. > > I think the original purpose of these test was allowing the user to > disable the service by chmod'ing -x the executable file. We discussed > a similar situation in > > https://patchwork.ozlabs.org/patch/818897/ > > [which by the way is still in my endless to-do list] Well, if you look at my feedback on this patch, I was already saying that the test to check whether radvd was executable or not was not useful. I don't think making a binary program non-executable as a way of disabling its startup at boot time is very common. Probably a ENABLED=true/false variable in /etc/default/<service> is more common. So, I think we should remove all those [ -x /usr/sbin/<foo> ] checks in our init scripts. Cc'ing Peter, Arnout and Yann to gather additional opinions on this. Thomas
Thomas, All, On 2018-04-15 20:18 +0200, Thomas Petazzoni spake thusly: > On Sun, 15 Apr 2018 12:31:49 -0300 (BRT), Carlos Santos wrote: > > >> +MODEMMANAGER_BIN=/usr/sbin/ModemManager > > >> PIDFILE=/var/run/ModemManager.pid > > >> > > >> [ -x $MODEMMANAGER_BIN ] || exit 0 > > > > > > In fact, I think this test doesn't make sense. It just exits without > > > any error if the binary doesn't exists, which is silly. > > > > > > I'm advocating for removing such tests altogether, which would make the > > > MODEMMANAGER_BIN variable unnecessary. > > > > I think the original purpose of these test was allowing the user to > > disable the service by chmod'ing -x the executable file. We discussed > > a similar situation in > > > > https://patchwork.ozlabs.org/patch/818897/ > > > > [which by the way is still in my endless to-do list] > > Well, if you look at my feedback on this patch, I was already saying > that the test to check whether radvd was executable or not was not > useful. > > I don't think making a binary program non-executable as a way of > disabling its startup at boot time is very common. Probably a > ENABLED=true/false variable in /etc/default/<service> is more common. > > So, I think we should remove all those [ -x /usr/sbin/<foo> ] checks in > our init scripts. > > Cc'ing Peter, Arnout and Yann to gather additional opinions on this. I agree with Thomas. This is a position we already stated quite a few times in the past. If one does not want the daemon, then just remove it and the init script from a post-build script. If it is about debugging, just remove the startup script on the target, or just 'exit 0' at the top of it. Regards, Yann E. MORIN.
> From: "Yann E. MORIN" <yann.morin.1998@free.fr> > To: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com> > Cc: "Carlos Santos" <casantos@datacom.ind.br>, "buildroot" <buildroot@buildroot.org>, "Aleksander Morgado" > <aleksander@aleksander.es>, "Peter Korsgaard" <peter@korsgaard.com>, "Arnout Vandecappelle" <arnout@mind.be> > Sent: Sunday, April 15, 2018 3:58:28 PM > Subject: Re: [Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script > Thomas, All, > > On 2018-04-15 20:18 +0200, Thomas Petazzoni spake thusly: >> On Sun, 15 Apr 2018 12:31:49 -0300 (BRT), Carlos Santos wrote: >> > >> +MODEMMANAGER_BIN=/usr/sbin/ModemManager >> > >> PIDFILE=/var/run/ModemManager.pid >> > >> >> > >> [ -x $MODEMMANAGER_BIN ] || exit 0 >> > > >> > > In fact, I think this test doesn't make sense. It just exits without >> > > any error if the binary doesn't exists, which is silly. >> > > >> > > I'm advocating for removing such tests altogether, which would make the >> > > MODEMMANAGER_BIN variable unnecessary. >> > >> > I think the original purpose of these test was allowing the user to >> > disable the service by chmod'ing -x the executable file. We discussed >> > a similar situation in >> > >> > https://patchwork.ozlabs.org/patch/818897/ >> > >> > [which by the way is still in my endless to-do list] >> >> Well, if you look at my feedback on this patch, I was already saying >> that the test to check whether radvd was executable or not was not >> useful. >> >> I don't think making a binary program non-executable as a way of >> disabling its startup at boot time is very common. Probably a >> ENABLED=true/false variable in /etc/default/<service> is more common. >> >> So, I think we should remove all those [ -x /usr/sbin/<foo> ] checks in >> our init scripts. >> >> Cc'ing Peter, Arnout and Yann to gather additional opinions on this. > > I agree with Thomas. > > This is a position we already stated quite a few times in the past. > > If one does not want the daemon, then just remove it and the init script > from a post-build script. If it is about debugging, just remove the > startup script on the target, or just 'exit 0' at the top of it. So I will prepare a series removing the tests from the startup scripts.
On 15-04-18 22:22, Carlos Santos wrote: >> From: "Yann E. MORIN" <yann.morin.1998@free.fr> >> To: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com> >> Cc: "Carlos Santos" <casantos@datacom.ind.br>, "buildroot" <buildroot@buildroot.org>, "Aleksander Morgado" >> <aleksander@aleksander.es>, "Peter Korsgaard" <peter@korsgaard.com>, "Arnout Vandecappelle" <arnout@mind.be> >> Sent: Sunday, April 15, 2018 3:58:28 PM >> Subject: Re: [Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script > >> Thomas, All, >> >> On 2018-04-15 20:18 +0200, Thomas Petazzoni spake thusly: >>> On Sun, 15 Apr 2018 12:31:49 -0300 (BRT), Carlos Santos wrote: >>>>>> +MODEMMANAGER_BIN=/usr/sbin/ModemManager >>>>>> PIDFILE=/var/run/ModemManager.pid >>>>>> >>>>>> [ -x $MODEMMANAGER_BIN ] || exit 0 >>>>> >>>>> In fact, I think this test doesn't make sense. It just exits without >>>>> any error if the binary doesn't exists, which is silly. >>>>> >>>>> I'm advocating for removing such tests altogether, which would make the >>>>> MODEMMANAGER_BIN variable unnecessary. >>>> >>>> I think the original purpose of these test was allowing the user to >>>> disable the service by chmod'ing -x the executable file. We discussed >>>> a similar situation in >>>> >>>> https://patchwork.ozlabs.org/patch/818897/ >>>> >>>> [which by the way is still in my endless to-do list] >>> >>> Well, if you look at my feedback on this patch, I was already saying >>> that the test to check whether radvd was executable or not was not >>> useful. >>> >>> I don't think making a binary program non-executable as a way of >>> disabling its startup at boot time is very common. Probably a >>> ENABLED=true/false variable in /etc/default/<service> is more common. >>> >>> So, I think we should remove all those [ -x /usr/sbin/<foo> ] checks in >>> our init scripts. >>> >>> Cc'ing Peter, Arnout and Yann to gather additional opinions on this. >> >> I agree with Thomas. +1 >> This is a position we already stated quite a few times in the past. >> >> If one does not want the daemon, then just remove it and the init script >> from a post-build script. If it is about debugging, just remove the >> startup script on the target, or just 'exit 0' at the top of it. > > So I will prepare a series removing the tests from the startup scripts. Well, part of the position we already stated a few times in the past is that we should make our sysvinit scripts conform to a pattern. So I think the optimal approach is: 1. Update the sysvinit scripts of one or two packages to conform to this pattern. Note that we have not really discussed the actual pattern, just a few preferences (e.g. don't test for the existence of the daemon). So it is important to update only one or two scripts so we can get convergence on the desired pattern, before updating many more packages. 2. Document this pattern in the manual. For me, it would be sufficient to refer to an existing sysvinit script rather than repeating it in the manual. 3. Update more packages. 4. Add some support to check-package to identify common mistakes in the pattern. Regards, Arnout
Hello, On Mon, 16 Apr 2018 17:53:25 +0200, Arnout Vandecappelle wrote: > > So I will prepare a series removing the tests from the startup scripts. > > Well, part of the position we already stated a few times in the past is that we > should make our sysvinit scripts conform to a pattern. So I think the optimal > approach is: > > 1. Update the sysvinit scripts of one or two packages to conform to this > pattern. Note that we have not really discussed the actual pattern, just a few > preferences (e.g. don't test for the existence of the daemon). So it is > important to update only one or two scripts so we can get convergence on the > desired pattern, before updating many more packages. > > 2. Document this pattern in the manual. For me, it would be sufficient to refer > to an existing sysvinit script rather than repeating it in the manual. > > 3. Update more packages. > > 4. Add some support to check-package to identify common mistakes in the pattern. Agreed on this plan. Anybody interested in this effort ? Best regards, Thomas
> From: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com> > To: "Arnout Vandecappelle" <arnout@mind.be> > Cc: "Carlos Santos" <casantos@datacom.ind.br>, "Yann Morin" <yann.morin.1998@free.fr>, "buildroot" > <buildroot@buildroot.org>, "Aleksander Morgado" <aleksander@aleksander.es>, "Peter Korsgaard" <peter@korsgaard.com> > Sent: Monday, May 14, 2018 3:57:06 AM > Subject: Re: [Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script > Hello, > > On Mon, 16 Apr 2018 17:53:25 +0200, Arnout Vandecappelle wrote: > >> > So I will prepare a series removing the tests from the startup scripts. >> >> Well, part of the position we already stated a few times in the past is that we >> should make our sysvinit scripts conform to a pattern. So I think the optimal >> approach is: >> >> 1. Update the sysvinit scripts of one or two packages to conform to this >> pattern. Note that we have not really discussed the actual pattern, just a few >> preferences (e.g. don't test for the existence of the daemon). So it is >> important to update only one or two scripts so we can get convergence on the >> desired pattern, before updating many more packages. >> >> 2. Document this pattern in the manual. For me, it would be sufficient to refer >> to an existing sysvinit script rather than repeating it in the manual. >> >> 3. Update more packages. >> >> 4. Add some support to check-package to identify common mistakes in the pattern. > > Agreed on this plan. > > Anybody interested in this effort ? I already have some scratches and will send some patches as soon as possible. <o/
diff --git a/package/modem-manager/S44modem-manager b/package/modem-manager/S44modem-manager index cf6d89601d..05803fe198 100755 --- a/package/modem-manager/S44modem-manager +++ b/package/modem-manager/S44modem-manager @@ -3,6 +3,7 @@ # Starts ModemManager # +MODEMMANAGER_BIN=/usr/sbin/ModemManager PIDFILE=/var/run/ModemManager.pid [ -x $MODEMMANAGER_BIN ] || exit 0 @@ -11,7 +12,7 @@ start() { printf "Starting ModemManager: " umask 077 start-stop-daemon -S -q -b -m -p $PIDFILE \ - --exec /usr/sbin/ModemManager + --exec $MODEMMANAGER_BIN [ $? = 0 ] && echo "OK" || echo "FAIL" } stop() {
MODEMMANAGER_BIN was never set. Fortunately it did not break the script execution due to the permissive behavior of "test": $ test -x non-existing || echo error error $ test -x || echo error test -x && echo success success Signed-off-by: Carlos Santos <casantos@datacom.ind.br> --- package/modem-manager/S44modem-manager | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)