[Buildroot] [PATCH 0/2] Fix/improve modem-manager init script

[Buildroot] [PATCH 0/2] Fix/improve modem-manager init script

[Carlos Santos]

- Initialize the MODEMMANAGER_BIN variable
- Allow a few customizations from a config file

Carlos Santos (2):
  modem-manager: fix test of unset variable in init script
  modem-manager: support a defaults file in the init script

 package/modem-manager/S44modem-manager | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.14.3

[Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script

[Carlos Santos]

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
@@ -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() {
-- 
2.14.3

[Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script

[Carlos Santos]

> 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]

-- 
Carlos Santos (Casantos) - DATACOM, P&D
?The greatest triumph that modern PR can offer is the transcendent 
success of having your words and actions judged by your reputation, 
rather than the other way about.? ? Christopher Hitchens

[Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script

[Thomas Petazzoni]

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 Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script

[Yann E. MORIN]

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.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script

[Carlos Santos]

> 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.

-- 
Carlos Santos (Casantos) - DATACOM, P&D
?The greatest triumph that modern PR can offer is the transcendent 
success of having your words and actions judged by your reputation, 
rather than the other way about.? ? Christopher Hitchens

[Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script

[Arnout Vandecappelle]

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


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/2] modem-manager: fix test of unset variable in init script

[Carlos Santos]

> 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/

-- 
Carlos Santos (Casantos) - DATACOM, P&D
?Marched towards the enemy, spear upright, armed with the certainty
that only the ignorant can have.? ? Epitaph of a volunteer

[Buildroot] [PATCH 2/2] modem-manager: support a defaults file in the init script

[Carlos Santos]

Sometimes it is useful to pass some parameters to ModemManager when it
starts (e.g. --log-level). Allow the user add a file with such flags in
a MODEMMANAGER_ARGS variable. This is simpler than overriding the whole
startup script (e.g. by means of a rootfs overlay).

Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
---
 package/modem-manager/S44modem-manager | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/package/modem-manager/S44modem-manager b/package/modem-manager/S44modem-manager
index 05803fe198..8fa5d0a429 100755
--- a/package/modem-manager/S44modem-manager
+++ b/package/modem-manager/S44modem-manager
@@ -3,6 +3,9 @@
 # Starts ModemManager
 #
 
+# Allow a few customizations from a config file
+test -r /etc/default/ModemManager && . /etc/default/ModemManager
+
 MODEMMANAGER_BIN=/usr/sbin/ModemManager
 PIDFILE=/var/run/ModemManager.pid
 
@@ -12,7 +15,7 @@ start() {
 	printf "Starting ModemManager: "
 	umask 077
 	start-stop-daemon -S -q -b -m -p $PIDFILE \
-		--exec $MODEMMANAGER_BIN
+		--exec $MODEMMANAGER_BIN -- $MODEMMANAGER_ARGS
 	[ $? = 0 ] && echo "OK" || echo "FAIL"
 }
 stop() {
-- 
2.14.3