Fix/improve modem-manager init script

PATCH New Carlos Santos 4 participants 11 messages Apr 13, 2018 – May 14, 2018

lore

Share & tools

lore

Carlos Santos

author

2018-04-13 16:25 UTC #0 lore raw

cover Fix/improve modem-manager init script

- 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(-)

Signature · 1 line

2.14.3

Carlos Santos

author

2018-04-13 16:25 UTC #1 lore raw

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

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 <[email protected]>

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

package/modem-manager/S44modem-manager +2 -1

index cf6d89601d..05803fe198 100755
@@ -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() {

Signature · 1 line

2.14.3

Thomas Petazzoni

2018-04-14 16:07 UTC #2 lore raw

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

Hello,

Quoted 19 lines · 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 <[email protected]>
---
 package/modem-manager/S44modem-manager | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
package/modem-manager/S44modem-manager +0 -0
index cf6d89601d..05803fe198 100755

package/modem-manager/S44modem-manager line 9

Quoted 7 lines

package/modem-manager/S44modem-manager +1 -0

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

Signature · 3 lines

Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Carlos Santos

author

2018-04-15 15:31 UTC #3 lore raw

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

Quoted 35 lines

From: "Thomas Petazzoni" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: "buildroot" <[email protected]>, "Aleksander Morgado" <[email protected]>
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,
Quoted 26 lines · 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 <[email protected]>
---
 package/modem-manager/S44modem-manager | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
package/modem-manager/S44modem-manager +1 -0
index cf6d89601d..05803fe198 100755
@@ -3,6 +3,7 @@
 # Starts ModemManager
 #
 
+MODEMMANAGER_BIN=/usr/sbin/ModemManager
 PIDFILE=/var/run/ModemManager.pid
 

Quoted 7 lines

package/modem-manager/S44modem-manager line 9
Quoted 1 line
package/modem-manager/S44modem-manager +0 -0
 [ -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]

Signature · 4 lines

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

Thomas Petazzoni

2018-04-15 18:18 UTC #4 lore raw

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

Hello,

Quoted 11 lines · Carlos Santos

Quoted 10 lines
Quoted 4 lines
+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.

Quoted 7 lines

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

Signature · 3 lines

Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Yann E. MORIN

2018-04-15 18:58 UTC #5 lore raw

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:

Quoted 24 lines · Carlos Santos

Quoted 11 lines
Quoted 10 lines
Quoted 4 lines
+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.
Quoted 7 lines
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.

Quoted 8 lines

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.

Signature · 6 lines

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

Carlos Santos

author

2018-04-15 20:22 UTC #6 lore raw

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

Quoted 43 lines

From: "Yann E. MORIN" <[email protected]>
To: "Thomas Petazzoni" <[email protected]>
Cc: "Carlos Santos" <[email protected]>, "buildroot" <[email protected]>, "Aleksander Morgado" <[email protected]>, "Peter Korsgaard" <[email protected]>, "Arnout Vandecappelle" <[email protected]>
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:
Quoted 24 lines · Carlos Santos
Quoted 11 lines
Quoted 10 lines
Quoted 4 lines
+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.
Quoted 7 lines
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.
Quoted 8 lines
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.

Quoted 7 lines

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.

Signature · 4 lines

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

Arnout Vandecappelle

2018-04-16 15:53 UTC #7 lore raw

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

Quoted 38 lines · Carlos Santos

Quoted 38 lines
From: "Yann E. MORIN" <[email protected]>
To: "Thomas Petazzoni" <[email protected]>
Cc: "Carlos Santos" <[email protected]>, "buildroot" <[email protected]>, "Aleksander Morgado" <[email protected]>, "Peter Korsgaard" <[email protected]>, "Arnout Vandecappelle" <[email protected]>
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:
Quoted 28 lines · Carlos Santos
Quoted 11 lines
Quoted 10 lines
Quoted 4 lines
+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.
Quoted 7 lines
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.

Quoted 6 lines

Quoted 6 lines
Quoted 4 lines
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.

Quoted 7 lines

Quoted 5 lines
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

Signature · 6 lines

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

Thomas Petazzoni

2018-05-14 06:57 UTC #8 lore raw

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

Hello,

Quoted 12 lines · Arnout Vandecappelle

Quoted 1 line
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.

Quoted 6 lines

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

Signature · 3 lines

Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Carlos Santos

author

2018-05-14 11:21 UTC #9 lore raw

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

Quoted 26 lines

From: "Thomas Petazzoni" <[email protected]>
To: "Arnout Vandecappelle" <[email protected]>
Cc: "Carlos Santos" <[email protected]>, "Yann Morin" <[email protected]>, "buildroot" <[email protected]>, "Aleksander Morgado" <[email protected]>, "Peter Korsgaard" <[email protected]>
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,
Quoted 15 lines · Arnout Vandecappelle
Quoted 1 line
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.

Quoted 7 lines

Quoted 3 lines
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/

Signature · 3 lines

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

Carlos Santos

author

2018-04-13 16:25 UTC #10 lore raw

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

In reply to 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 <[email protected]>

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

package/modem-manager/S44modem-manager +4 -1

index 05803fe198..8fa5d0a429 100755
@@ -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() {

Signature · 1 line

2.14.3