diff mbox

[v2,3/3] rpi-userland: Fix vcfiled startup

Message ID 1407332350-27220-3-git-send-email-benoit.thebaudeau@advansee.com
State Superseded
Headers show

Commit Message

Benoît Thébaudeau Aug. 6, 2014, 1:39 p.m. UTC 
The VideoCore file server daemon startup script installed from this package is
not compatible with BuildRoot (because of its naming and other Debian
dependencies), which prevented vcfiled from starting. Hence, prevent this
package from installing its vcfiled startup script, and install a vcfiled SysV
init script suitable for BuildRoot.

Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>

---

Changes in v2:
 - Indent .mk rules with tabs.
 - Simplify the new vcfiled startup script.
 - Rename S97vcfiled to S94vcfiled for a better integration with existing init
   scripts.
---
 package/rpi-userland/S94vcfiled      | 47 ++++++++++++++++++++++++++++++++++++
 package/rpi-userland/rpi-userland.mk |  8 ++++++
 2 files changed, 55 insertions(+)
 create mode 100755 package/rpi-userland/S94vcfiled

Comments

Yann E. MORIN Aug. 6, 2014, 5:33 p.m. UTC | #1 
Benoit, All,

On 2014-08-06 15:39 +0200, Benoît Thébaudeau spake thusly:
> The VideoCore file server daemon startup script installed from this package is
> not compatible with BuildRoot (because of its naming and other Debian
> dependencies), which prevented vcfiled from starting. Hence, prevent this
> package from installing its vcfiled startup script, and install a vcfiled SysV
> init script suitable for BuildRoot.

I'm not sure I would be happy that the GPU is allowed uncontrolled
access to the filesystem.

Please, make this an option, defaulting to 'n', so the user can
willingly choose to install it or not.

Otherwise, see below...

> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
[--SNIP--]
> diff --git a/package/rpi-userland/S94vcfiled b/package/rpi-userland/S94vcfiled
> new file mode 100755
> index 0000000..25a0fcd
> --- /dev/null
> +++ b/package/rpi-userland/S94vcfiled
> @@ -0,0 +1,47 @@
> +#!/bin/sh
> +
> +NAME=vcfiled
> +DESC="VideoCore file server daemon $NAME"
> +DAEMON="/usr/sbin/$NAME"
> +DAEMON_ARGS=""
> +CFGFILE="/etc/default/$NAME"
> +PIDFILE="/var/run/$NAME/$NAME"

Are you sure about the path to the PID file?
What about:
    PIDFILE="/var/run/$NAME.pid"

Othwerwise, looks good after a casual look. Ditto your previous patches,
I'll try to find some time tonight to test it.

Regards,
Yann E. MORIN.

> +# Read configuration variable file if it is present
> +[ -r "$CFGFILE" ] && . "$CFGFILE"
> +
> +do_start()
> +{
> +	echo -n "Starting $DESC: "
> +	start-stop-daemon -S -q -p "$PIDFILE" -x "$DAEMON" -- $DAEMON_ARGS &&
> +			echo "done" || echo "failed"
> +}
> +
> +do_stop()
> +{
> +	echo -n "Stopping $DESC: "
> +	if start-stop-daemon -K -q -R TERM/30/KILL/5 -p "$PIDFILE" -n "$NAME"; then
> +		# This daemon does not remove its PID file when it exits.
> +		rm -f "$PIDFILE"
> +		echo "done"
> +	else
> +		echo "failed"
> +	fi
> +}
> +
> +case "$1" in
> +	start)
> +		do_start
> +		;;
> +	stop)
> +		do_stop
> +		;;
> +	restart|reload)
> +		do_stop
> +		do_start
> +		;;
> +	*)
> +		echo "Usage: $0 {start|stop|restart|reload}" >&2
> +		exit 1
> +		;;
> +esac
> diff --git a/package/rpi-userland/rpi-userland.mk b/package/rpi-userland/rpi-userland.mk
> index 81ed95c..0d29f24 100644
> --- a/package/rpi-userland/rpi-userland.mk
> +++ b/package/rpi-userland/rpi-userland.mk
> @@ -13,7 +13,15 @@ RPI_USERLAND_CONF_OPT = -DVMCS_INSTALL_PREFIX=/usr
>  
>  RPI_USERLAND_PROVIDES = libegl libgles libopenmax libopenvg
>  
> +define RPI_USERLAND_INSTALL_INIT_SYSV
> +	$(INSTALL) -m 0755 -D package/rpi-userland/S94vcfiled \
> +			$(TARGET_DIR)/etc/init.d/S94vcfiled
> +endef
> +
>  define RPI_USERLAND_POST_TARGET_CLEANUP
> +	rm -f $(TARGET_DIR)/etc/init.d/vcfiled
> +	rm -f $(TARGET_DIR)/usr/share/install/vcfiled
> +	rmdir --ignore-fail-on-non-empty $(TARGET_DIR)/usr/share/install
>  	rm -Rf $(TARGET_DIR)/usr/src
>  endef
>  RPI_USERLAND_POST_INSTALL_TARGET_HOOKS += RPI_USERLAND_POST_TARGET_CLEANUP
> -- 
> 1.9.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Benoît Thébaudeau Aug. 6, 2014, 5:40 p.m. UTC | #2 
Dear Yann E. MORIN,

On Wednesday, August 6, 2014 7:33:51 PM, Yann E. MORIN wrote:
> Benoit, All,
> 
> On 2014-08-06 15:39 +0200, Benoît Thébaudeau spake thusly:
> > The VideoCore file server daemon startup script installed from this package
> > is
> > not compatible with BuildRoot (because of its naming and other Debian
> > dependencies), which prevented vcfiled from starting. Hence, prevent this
> > package from installing its vcfiled startup script, and install a vcfiled
> > SysV
> > init script suitable for BuildRoot.
> 
> I'm not sure I would be happy that the GPU is allowed uncontrolled
> access to the filesystem.
> 
> Please, make this an option, defaulting to 'n', so the user can
> willingly choose to install it or not.

Will do.

> Otherwise, see below...
> 
> > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> [--SNIP--]
> > diff --git a/package/rpi-userland/S94vcfiled
> > b/package/rpi-userland/S94vcfiled
> > new file mode 100755
> > index 0000000..25a0fcd
> > --- /dev/null
> > +++ b/package/rpi-userland/S94vcfiled
> > @@ -0,0 +1,47 @@
> > +#!/bin/sh
> > +
> > +NAME=vcfiled
> > +DESC="VideoCore file server daemon $NAME"
> > +DAEMON="/usr/sbin/$NAME"
> > +DAEMON_ARGS=""
> > +CFGFILE="/etc/default/$NAME"
> > +PIDFILE="/var/run/$NAME/$NAME"
> 
> Are you sure about the path to the PID file?
> What about:
>     PIDFILE="/var/run/$NAME.pid"

Yes, sure, this is the path used by vcfiled, which creates its PID file itself
rather than relying on start-stop-daemon -m for that.

> Othwerwise, looks good after a casual look. Ditto your previous patches,
> I'll try to find some time tonight to test it.

Thanks.

[...]

Regards,
Benoît
Benoît Thébaudeau Aug. 6, 2014, 7:56 p.m. UTC | #3 
Dear Yann E. MORIN,

On Wed, Aug 6, 2014 at 7:40 PM, Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:
> Dear Yann E. MORIN,
>
> On Wednesday, August 6, 2014 7:33:51 PM, Yann E. MORIN wrote:
>> Benoit, All,
>>
>> On 2014-08-06 15:39 +0200, Benoît Thébaudeau spake thusly:

[...]

>> Otherwise, see below...
>>
>> > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
>> [--SNIP--]
>> > diff --git a/package/rpi-userland/S94vcfiled
>> > b/package/rpi-userland/S94vcfiled
>> > new file mode 100755
>> > index 0000000..25a0fcd
>> > --- /dev/null
>> > +++ b/package/rpi-userland/S94vcfiled
>> > @@ -0,0 +1,47 @@
>> > +#!/bin/sh
>> > +
>> > +NAME=vcfiled
>> > +DESC="VideoCore file server daemon $NAME"
>> > +DAEMON="/usr/sbin/$NAME"
>> > +DAEMON_ARGS=""
>> > +CFGFILE="/etc/default/$NAME"
>> > +PIDFILE="/var/run/$NAME/$NAME"
>>
>> Are you sure about the path to the PID file?
>> What about:
>>     PIDFILE="/var/run/$NAME.pid"
>
> Yes, sure, this is the path used by vcfiled, which creates its PID file itself
> rather than relying on start-stop-daemon -m for that.

I have checked the code, and this path can be overridden by defining
VCFILED_LOCKFILE, so I can set it to '/var/run/vcfiled.pid' if you
really prefer. It's up to you. You tell me.

[...]

Regards,
Benoît
Yann E. MORIN Aug. 6, 2014, 8:02 p.m. UTC | #4 
Benoit, All,

On 2014-08-06 21:56 +0200, Benoît Thébaudeau spake thusly:
> > On Wednesday, August 6, 2014 7:33:51 PM, Yann E. MORIN wrote:
> >> On 2014-08-06 15:39 +0200, Benoît Thébaudeau spake thusly:
> >> > +PIDFILE="/var/run/$NAME/$NAME"
> >>
> >> Are you sure about the path to the PID file?
> >> What about:
> >>     PIDFILE="/var/run/$NAME.pid"
[--SNIP--]
> I have checked the code, and this path can be overridden by defining
> VCFILED_LOCKFILE, so I can set it to '/var/run/vcfiled.pid' if you
> really prefer. It's up to you. You tell me.

Well, I think it would be better, yes. There's no need to create a
directory just to put the PID file.

Thanks!

Regards,
Yann E. MORIN.
diff mbox

Patch

diff --git a/package/rpi-userland/S94vcfiled b/package/rpi-userland/S94vcfiled
new file mode 100755
index 0000000..25a0fcd
--- /dev/null
+++ b/package/rpi-userland/S94vcfiled
@@ -0,0 +1,47 @@ 
+#!/bin/sh
+
+NAME=vcfiled
+DESC="VideoCore file server daemon $NAME"
+DAEMON="/usr/sbin/$NAME"
+DAEMON_ARGS=""
+CFGFILE="/etc/default/$NAME"
+PIDFILE="/var/run/$NAME/$NAME"
+
+# Read configuration variable file if it is present
+[ -r "$CFGFILE" ] && . "$CFGFILE"
+
+do_start()
+{
+	echo -n "Starting $DESC: "
+	start-stop-daemon -S -q -p "$PIDFILE" -x "$DAEMON" -- $DAEMON_ARGS &&
+			echo "done" || echo "failed"
+}
+
+do_stop()
+{
+	echo -n "Stopping $DESC: "
+	if start-stop-daemon -K -q -R TERM/30/KILL/5 -p "$PIDFILE" -n "$NAME"; then
+		# This daemon does not remove its PID file when it exits.
+		rm -f "$PIDFILE"
+		echo "done"
+	else
+		echo "failed"
+	fi
+}
+
+case "$1" in
+	start)
+		do_start
+		;;
+	stop)
+		do_stop
+		;;
+	restart|reload)
+		do_stop
+		do_start
+		;;
+	*)
+		echo "Usage: $0 {start|stop|restart|reload}" >&2
+		exit 1
+		;;
+esac
diff --git a/package/rpi-userland/rpi-userland.mk b/package/rpi-userland/rpi-userland.mk
index 81ed95c..0d29f24 100644
--- a/package/rpi-userland/rpi-userland.mk
+++ b/package/rpi-userland/rpi-userland.mk
@@ -13,7 +13,15 @@  RPI_USERLAND_CONF_OPT = -DVMCS_INSTALL_PREFIX=/usr
 
 RPI_USERLAND_PROVIDES = libegl libgles libopenmax libopenvg
 
+define RPI_USERLAND_INSTALL_INIT_SYSV
+	$(INSTALL) -m 0755 -D package/rpi-userland/S94vcfiled \
+			$(TARGET_DIR)/etc/init.d/S94vcfiled
+endef
+
 define RPI_USERLAND_POST_TARGET_CLEANUP
+	rm -f $(TARGET_DIR)/etc/init.d/vcfiled
+	rm -f $(TARGET_DIR)/usr/share/install/vcfiled
+	rmdir --ignore-fail-on-non-empty $(TARGET_DIR)/usr/share/install
 	rm -Rf $(TARGET_DIR)/usr/src
 endef
 RPI_USERLAND_POST_INSTALL_TARGET_HOOKS += RPI_USERLAND_POST_TARGET_CLEANUP