[1/1] rpi-userland: Fix vcfiled startup

The VideoCore file server daemon SysV 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
startup script suitable for BuildRoot.

Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
 package/rpi-userland/S97vcfiled      | 100 +++++++++++++++++++++++++++++++++++
 package/rpi-userland/rpi-userland.mk |   8 +++
 2 files changed, 108 insertions(+)
 create mode 100755 package/rpi-userland/S97vcfiled

Dear Benoît Thébaudeau,

On Mon,  4 Aug 2014 20:20:56 +0200, Benoît Thébaudeau wrote:
> The VideoCore file server daemon SysV 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
> startup script suitable for BuildRoot.
> 
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>

Thanks for this patch. Experts in rpi stuff will comment, but I have
one question below:


> diff --git a/package/rpi-userland/S97vcfiled b/package/rpi-userland/S97vcfiled
> new file mode 100755
> index 0000000..87c4e76
> --- /dev/null
> +++ b/package/rpi-userland/S97vcfiled
> @@ -0,0 +1,100 @@
> +#! /bin/sh
> +### BEGIN INIT INFO
> +# Provides:          vcfiled
> +# Required-Start:    udev
> +# Required-Stop:     udev
> +# Short-Description: VideoCore file server daemon
> +### END INIT INFO

I don't think we need this header in Buildroot, since neither the
Busybox init nor the sysvinit are making any use of it.

However, it indicates a dependency on udev, while the rpi-userland
package certainly does not depend on udev.

Also, this daemon was until not running on any rpi configuration
according to what you said. So what it is useful for? Should it be
mandatory for all rpi-userland installations, or optional?

Thanks!

Thomas

Benoit, All,

On 2014-08-04 20:20 +0200, Benoît Thébaudeau spake thusly:
> The VideoCore file server daemon SysV 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
> startup script suitable for BuildRoot.
> 
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>

In addition to Thomas' comments, here are mines:

> diff --git a/package/rpi-userland/S97vcfiled b/package/rpi-userland/S97vcfiled
> new file mode 100755
> index 0000000..87c4e76
> --- /dev/null
> +++ b/package/rpi-userland/S97vcfiled
> @@ -0,0 +1,100 @@
> +#! /bin/sh
> +### BEGIN INIT INFO
> +# Provides:          vcfiled
> +# Required-Start:    udev
> +# Required-Stop:     udev
> +# Short-Description: VideoCore file server daemon
> +### END INIT INFO
> +
> +DESC="VideoCore file server daemon"
> +NAME=vcfiled
> +VCROOT=/usr
> +DAEMON=$VCROOT/sbin/$NAME
> +DAEMON_ARGS=""
> +PIDFILE=/var/run/$NAME/$NAME
> +SCRIPTNAME="$0"
> +
> +# Exit if the package is not installed
> +[ -x "$DAEMON" ] || exit 0
> +
> +# Read configuration variable file if it is present
> +[ -r /etc/default/$NAME ] && . /etc/default/$NAME
> +
> +#
> +# Function that starts the daemon/service
> +#
> +do_start()
> +{
> +	# Return
> +	#   0 if daemon has been started
> +	#   1 if daemon was already running
> +	#   2 if daemon could not be started
> +	start-stop-daemon --stop --quiet --pidfile $PIDFILE --exec $DAEMON --test > /dev/null \
> +		&& return 1
> +	start-stop-daemon --start --quiet --pidfile $PIDFILE --exec $DAEMON -- \
> +		$DAEMON_ARGS \
> +		|| return 2
> +}
> +
> +#
> +# Function that stops the daemon/service
> +#
> +do_stop()
> +{
> +	# Return
> +	#   0 if daemon has been stopped
> +	#   1 if daemon was already stopped
> +	#   2 if daemon could not be stopped
> +	#   other if a failure occurred
> +	start-stop-daemon --stop --quiet --retry=TERM/30/KILL/5 --pidfile $PIDFILE --name $NAME
> +	RETVAL="$?"
> +	[ "$RETVAL" = 2 ] && return 2
> +	start-stop-daemon --stop --quiet --oknodo --retry=0/30/KILL/5 --exec $DAEMON
> +	[ "$?" = 2 ] && return 2
> +	# Many daemons don't delete their pidfiles when they exit.
> +	rm -f $PIDFILE
> +	return "$RETVAL"
> +}
> +
> +case "$1" in
> +  start)
> +	echo -n "Starting $DESC $NAME: "
> +	do_start
> +	case "$?" in
> +		0|1) echo done ;;
> +		2) echo failed ;;
> +	esac
> +	;;
> +  stop)
> +	echo -n "Stopping $DESC $NAME: "
> +	do_stop
> +	case "$?" in
> +		0|1) echo done ;;
> +		2) echo failed ;;
> +	esac
> +	;;
> +  restart|force-reload)
> +	echo -n "Restarting $DESC $NAME: "
> +	do_stop
> +	case "$?" in
> +	  0|1)
> +		do_start
> +		case "$?" in
> +			0) echo done ;;
> +			1) echo stop ignored ;; # Old process is still running
> +			*) echo start failed ;; # Failed to start
> +		esac
> +		;;
> +	  *)
> +		# Failed to stop
> +		echo stop failed
> +		;;
> +	esac
> +	;;
> +  *)
> +	echo "Usage: $SCRIPTNAME {start|stop|restart|force-reload}" >&2
> +	exit 3
> +	;;
> +esac
> +
> +:

Last line uneeded.

Also, I'd like to see a simpler script. There is no need to handle the
already-running case. There is nothing that will try to re-run the
daemon in the standard init scripts. For development, it's the user's
responsibility to stop-and-start the daemon.

Also, no need to handle the fail-to-stop case, we're gonna halt/reboot
anyway.

See for example: package/xbmc/S50xbmc

> diff --git a/package/rpi-userland/rpi-userland.mk b/package/rpi-userland/rpi-userland.mk
> index 717eab1..bdf4e91 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/S97vcfiled \

Lines should be tab-prefixed.

> +		$(TARGET_DIR)/etc/init.d/S97vcfiled
> +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

Ditto, tab-prefixed.

I'm not a fan of 'rmdir' (I got burnt by it long ago, and it still
hurts). But OK.

Regards,
Yann E. MORIN.

>      rm -Rf $(TARGET_DIR)/usr/src
>  endef
>  RPI_USERLAND_POST_INSTALL_TARGET_HOOKS += RPI_USERLAND_POST_TARGET_CLEANUP

Dear Thomas Petazzoni,

On Monday, August 4, 2014 8:33:52 PM, Thomas Petazzoni wrote:
> Dear Benoît Thébaudeau,
> 
> On Mon,  4 Aug 2014 20:20:56 +0200, Benoît Thébaudeau wrote:
> > The VideoCore file server daemon SysV 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
> > startup script suitable for BuildRoot.
> > 
> > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> 
> Thanks for this patch. Experts in rpi stuff will comment, but I have
> one question below:
> 
> 
> > diff --git a/package/rpi-userland/S97vcfiled
> > b/package/rpi-userland/S97vcfiled
> > new file mode 100755
> > index 0000000..87c4e76
> > --- /dev/null
> > +++ b/package/rpi-userland/S97vcfiled
> > @@ -0,0 +1,100 @@
> > +#! /bin/sh
> > +### BEGIN INIT INFO
> > +# Provides:          vcfiled
> > +# Required-Start:    udev
> > +# Required-Stop:     udev
> > +# Short-Description: VideoCore file server daemon
> > +### END INIT INFO
> 
> I don't think we need this header in Buildroot, since neither the
> Busybox init nor the sysvinit are making any use of it.

Right. I had kept this script as close as possible to the original script from
the package, but this is indeed not needed. I will drop that and simplify this
script as per Yann's comments.

> However, it indicates a dependency on udev, while the rpi-userland
> package certainly does not depend on udev.

Nothing in rpi-userland needs udev at build time, but vcfiled needs to access
/dev/vchiq at runtime, so something has to bring up this node. Whether it is a
static /dev, devtmpfs, udev or something else does not matter, so I don't think
that a dependency to udev should be added here.

> Also, this daemon was until not running on any rpi configuration
> according to what you said. So what it is useful for? Should it be
> mandatory for all rpi-userland installations, or optional?

I'm not certain here. This daemon sets up an interface giving the GPU access to
the file systems, but I don't know what applications use that if any. On some
forums, omxplayer or the hello_pi examples from rpi-userland are said to require
vcfiled, but I've tested them, and I couldn't make them fail without vcfiled.

I would need an RPi expert to answer this. Perhaps there was such a requirement
in the past that is less used nowadays. Anyway, this daemon is still installed
and started on Raspbian, so BuildRoot should probably do the same, all the more
this is part of the rpi-userland installation rules.

Best regards,
Benoît