Patchwork [1/1] ser2net: add default init script

login
register
mail settings
Submitter Jean-Christophe PLAGNIOL-VILLARD
Date May 5, 2013, 12:43 p.m.
Message ID <1367757785-5850-1-git-send-email-plagnioj@jcrosoft.com>
Download mbox | patch
Permalink /patch/241511/
State Superseded
Headers show

Comments

Jean-Christophe PLAGNIOL-VILLARD - May 5, 2013, 12:43 p.m.
add option to specify the configuration file

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 package/ser2net/Config.in  |    9 +++++++++
 package/ser2net/S50ser2net |   48 ++++++++++++++++++++++++++++++++++++++++++++
 package/ser2net/ser2net.mk |   27 +++++++++++++++++++++++++
 3 files changed, 84 insertions(+)
 create mode 100644 package/ser2net/S50ser2net
Yann E. MORIN - May 5, 2013, 1:50 p.m.
Jean-Christophe, All,

See my comments inlined below.

On Sun, May 05, 2013 at 02:43:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> add option to specify the configuration file
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  package/ser2net/Config.in  |    9 +++++++++
>  package/ser2net/S50ser2net |   48 ++++++++++++++++++++++++++++++++++++++++++++
>  package/ser2net/ser2net.mk |   27 +++++++++++++++++++++++++
>  3 files changed, 84 insertions(+)
>  create mode 100644 package/ser2net/S50ser2net
> 
> diff --git a/package/ser2net/Config.in b/package/ser2net/Config.in
> index e3b8870..3480d85 100644
> --- a/package/ser2net/Config.in
> +++ b/package/ser2net/Config.in
> @@ -8,5 +8,14 @@ config BR2_PACKAGE_SER2NET
>  
>  	  http://ser2net.sourceforge.net
>  
> +if BR2_PACKAGE_SER2NET
> +
> +config BR2_PACKAGE_SER2NET_CONF
> +	string "Configuration file"
> +	help
> +	  Configuration file.

I think we should provide a default, basic configuration file that we
install unconditionnally to the rootfs. The user will then be free to
provide a post-build script to tweak that file.


> diff --git a/package/ser2net/ser2net.mk b/package/ser2net/ser2net.mk
> index 378dc06..5a2539e 100644
> --- a/package/ser2net/ser2net.mk
> +++ b/package/ser2net/ser2net.mk
> @@ -9,4 +9,31 @@ SER2NET_SITE = http://downloads.sourceforge.net/project/ser2net/ser2net
>  SER2NET_LICENSE = GPLv2+
>  SER2NET_LICENSE_FILES = COPYING
>  
> +define SER2NET_INIT_SCRIPT_INSTALL
> +	$(INSTALL) -m 0755 package/ser2net/S50ser2net $(TARGET_DIR)/etc/init.d
> +endef
> +
> +SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_INSTALL

Please use SER2NET_INSTALL_INIT_SYSV to install that script, eg.:
    SER2NET_INSTALL_INIT_SYSV += SER2NET_INIT_SCRIPT_INSTALL

this way, the init script will only be installed if a sysv init sequence
is used (and not systemd).

Also, would it be possible to provide a systemd unit?
Ditto, use: SER2NET_INSTALL_INIT_SYSTEMD

> +
> +define SER2NET_INIT_SCRIPT_UNINSTALL
> +	rm -f $(TARGET_DIR)/etc/init.d/S50ser2net
> +endef
> +
> +SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_UNINSTALL

I think we decided that uninstall targets are not needed, because
uninstall is not working completely in Buildroot. Drop it.

> +
> +define SER2NET_CONF_INSTALL
> +	if [ "$(BR2_PACKAGE_SER2NET_CONF)" != "" ] ; then \
> +		$(INSTALL) -m 0644 $(BR2_PACKAGE_SER2NET_CONF) \
> +			$(TARGET_DIR)/etc/ser2net.conf ; \
> +	fi
> +endef
> +
> +SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_CONF_INSTALL

Always install a default configuration file, as said above.

> +define SER2NET_CONFIG_UNINSTALL
> +	rm -f $(TARGET_DIR)/etc/ser2net.conf
> +endef
> +
> +SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_CONFIG_UNINSTALL

Ditto for uninstall: drop it.

Thank you!

Regards,
Yann E. MORIN.
Jean-Christophe PLAGNIOL-VILLARD - May 5, 2013, 2:32 p.m.
On 15:50 Sun 05 May     , Yann E. MORIN wrote:
> Jean-Christophe, All,
> 
> See my comments inlined below.
> 
> On Sun, May 05, 2013 at 02:43:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > add option to specify the configuration file
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> >  package/ser2net/Config.in  |    9 +++++++++
> >  package/ser2net/S50ser2net |   48 ++++++++++++++++++++++++++++++++++++++++++++
> >  package/ser2net/ser2net.mk |   27 +++++++++++++++++++++++++
> >  3 files changed, 84 insertions(+)
> >  create mode 100644 package/ser2net/S50ser2net
> > 
> > diff --git a/package/ser2net/Config.in b/package/ser2net/Config.in
> > index e3b8870..3480d85 100644
> > --- a/package/ser2net/Config.in
> > +++ b/package/ser2net/Config.in
> > @@ -8,5 +8,14 @@ config BR2_PACKAGE_SER2NET
> >  
> >  	  http://ser2net.sourceforge.net
> >  
> > +if BR2_PACKAGE_SER2NET
> > +
> > +config BR2_PACKAGE_SER2NET_CONF
> > +	string "Configuration file"
> > +	help
> > +	  Configuration file.
> 
> I think we should provide a default, basic configuration file that we
> install unconditionnally to the rootfs. The user will then be free to
> provide a post-build script to tweak that file.

I hate post-build it's not trackable and as it's completly board specific with
x serial to export I do not want a generic file
> 
> 
> > diff --git a/package/ser2net/ser2net.mk b/package/ser2net/ser2net.mk
> > index 378dc06..5a2539e 100644
> > --- a/package/ser2net/ser2net.mk
> > +++ b/package/ser2net/ser2net.mk
> > @@ -9,4 +9,31 @@ SER2NET_SITE = http://downloads.sourceforge.net/project/ser2net/ser2net
> >  SER2NET_LICENSE = GPLv2+
> >  SER2NET_LICENSE_FILES = COPYING
> >  
> > +define SER2NET_INIT_SCRIPT_INSTALL
> > +	$(INSTALL) -m 0755 package/ser2net/S50ser2net $(TARGET_DIR)/etc/init.d
> > +endef
> > +
> > +SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_INSTALL
> 
> Please use SER2NET_INSTALL_INIT_SYSV to install that script, eg.:
>     SER2NET_INSTALL_INIT_SYSV += SER2NET_INIT_SCRIPT_INSTALL
> 
> this way, the init script will only be installed if a sysv init sequence
> is used (and not systemd).
> 
> Also, would it be possible to provide a systemd unit?
> Ditto, use: SER2NET_INSTALL_INIT_SYSTEMD
> 
> > +
> > +define SER2NET_INIT_SCRIPT_UNINSTALL
> > +	rm -f $(TARGET_DIR)/etc/init.d/S50ser2net
> > +endef
> > +
> > +SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_UNINSTALL
> 
> I think we decided that uninstall targets are not needed, because
> uninstall is not working completely in Buildroot. Drop it.
> 
> > +
> > +define SER2NET_CONF_INSTALL
> > +	if [ "$(BR2_PACKAGE_SER2NET_CONF)" != "" ] ; then \
> > +		$(INSTALL) -m 0644 $(BR2_PACKAGE_SER2NET_CONF) \
> > +			$(TARGET_DIR)/etc/ser2net.conf ; \
> > +	fi
> > +endef
> > +
> > +SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_CONF_INSTALL
> 
> Always install a default configuration file, as said above.
> 
> > +define SER2NET_CONFIG_UNINSTALL
> > +	rm -f $(TARGET_DIR)/etc/ser2net.conf
> > +endef
> > +
> > +SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_CONFIG_UNINSTALL
> 
> Ditto for uninstall: drop it.
> 
> Thank you!
> 
> 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.  |
> '------------------------------^-------^------------------^--------------------'
Yann E. MORIN - May 5, 2013, 2:58 p.m.
Jean-Christophe, All,

On Sun, May 05, 2013 at 04:32:52PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 15:50 Sun 05 May     , Yann E. MORIN wrote:
> > Jean-Christophe, All,
> > 
> > See my comments inlined below.
> > 
> > On Sun, May 05, 2013 at 02:43:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > add option to specify the configuration file
> > > 
> > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > > ---
> > >  package/ser2net/Config.in  |    9 +++++++++
> > >  package/ser2net/S50ser2net |   48 ++++++++++++++++++++++++++++++++++++++++++++
> > >  package/ser2net/ser2net.mk |   27 +++++++++++++++++++++++++
> > >  3 files changed, 84 insertions(+)
> > >  create mode 100644 package/ser2net/S50ser2net
> > > 
> > > diff --git a/package/ser2net/Config.in b/package/ser2net/Config.in
> > > index e3b8870..3480d85 100644
> > > --- a/package/ser2net/Config.in
> > > +++ b/package/ser2net/Config.in
> > > @@ -8,5 +8,14 @@ config BR2_PACKAGE_SER2NET
> > >  
> > >  	  http://ser2net.sourceforge.net
> > >  
> > > +if BR2_PACKAGE_SER2NET
> > > +
> > > +config BR2_PACKAGE_SER2NET_CONF
> > > +	string "Configuration file"
> > > +	help
> > > +	  Configuration file.
> > 
> > I think we should provide a default, basic configuration file that we
> > install unconditionnally to the rootfs. The user will then be free to
> > provide a post-build script to tweak that file.
> 
> I hate post-build it's not trackable and as it's completly board specific with
> x serial to export I do not want a generic file

It's not about what *you* want, it's about what is *best* for Buildroot
and *all* its users, *and* is maintenable and easily reproduced (which
pointing to a config file outside of the buildroot tree is definitely
not) *and* can fit your use case (which what I suggest should).

We're speaking about a _default_ and _simple_ configuration file. Of
course it can not cover all use cases, but just exporting a single
serial port would be enough. Customisation beyond that is really part of
the post-build scripts.

And if you think that post-build script are not trackable, then letting
the user specify a config file (presumably out-side of the Buildroot tree)
is no more trackable either. So this excuse does not hold water.

And I *do* track my post-build and post-image scripts. Here's the
setting I use:

    .../buildroot/          <- the buildroot tree
    .../board-X/            <- the board-X tree, which *is* a git tree
    .../board-X/configs/    <- dir with all .config files for board-X
    .../board-X/scripts/    <- dir with add scripts for board-X
    .../board-X/misc/       <- misc files to push into rootfs
    .../board-X/build/      <- untracked dir for out-of-tree build

Then I have those files tracked in my git tree:
    .../board-X/configs/br.defconfig
    .../board-X/configs/linux.defconfig
    .../board-X/configs/bb.defconfig
    .../board-X/scripts/ssh-key-in-root.ssh.authorized_keys
    .../board-X/scripts/other-mount-points-in-etc.fstab
    .../board-X/scripts/create-sdcard-image
    .../board-X/scripts/checksum-images
    .../board-X/misc/S98dvb-devices
   [and so on...]

Now, all I said above is my point of view. Just let see what the
maintainer will accept. :-)

Regards,
Yann E. MORIN.
Jean-Christophe PLAGNIOL-VILLARD - May 5, 2013, 8:19 p.m.
On 16:58 Sun 05 May     , Yann E. MORIN wrote:
> Jean-Christophe, All,
> 
> On Sun, May 05, 2013 at 04:32:52PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 15:50 Sun 05 May     , Yann E. MORIN wrote:
> > > Jean-Christophe, All,
> > > 
> > > See my comments inlined below.
> > > 
> > > On Sun, May 05, 2013 at 02:43:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > add option to specify the configuration file
> > > > 
> > > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > > > ---
> > > >  package/ser2net/Config.in  |    9 +++++++++
> > > >  package/ser2net/S50ser2net |   48 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  package/ser2net/ser2net.mk |   27 +++++++++++++++++++++++++
> > > >  3 files changed, 84 insertions(+)
> > > >  create mode 100644 package/ser2net/S50ser2net
> > > > 
> > > > diff --git a/package/ser2net/Config.in b/package/ser2net/Config.in
> > > > index e3b8870..3480d85 100644
> > > > --- a/package/ser2net/Config.in
> > > > +++ b/package/ser2net/Config.in
> > > > @@ -8,5 +8,14 @@ config BR2_PACKAGE_SER2NET
> > > >  
> > > >  	  http://ser2net.sourceforge.net
> > > >  
> > > > +if BR2_PACKAGE_SER2NET
> > > > +
> > > > +config BR2_PACKAGE_SER2NET_CONF
> > > > +	string "Configuration file"
> > > > +	help
> > > > +	  Configuration file.
> > > 
> > > I think we should provide a default, basic configuration file that we
> > > install unconditionnally to the rootfs. The user will then be free to
> > > provide a post-build script to tweak that file.
> > 
> > I hate post-build it's not trackable and as it's completly board specific with
> > x serial to export I do not want a generic file
> 
> It's not about what *you* want, it's about what is *best* for Buildroot
> and *all* its users, *and* is maintenable and easily reproduced (which
> pointing to a config file outside of the buildroot tree is definitely
> not) *and* can fit your use case (which what I suggest should).

I want to put the ser2net.conf in the tree not our of tree
> 
> We're speaking about a _default_ and _simple_ configuration file. Of
> course it can not cover all use cases, but just exporting a single
> serial port would be enough. Customisation beyond that is really part of
> the post-build scripts.
the problem here which port you export?

the ttyS0? no it's mostly the default console

how could you even think of any generic file as the name of the port(s) and
configuration is board specific

This will be completly board specific

Best Regards,
J.
Yann E. MORIN - May 5, 2013, 9:23 p.m.
Jean-Christophe, All,

On Sun, May 05, 2013 at 10:19:32PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:58 Sun 05 May     , Yann E. MORIN wrote:
> > On Sun, May 05, 2013 at 04:32:52PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 15:50 Sun 05 May     , Yann E. MORIN wrote:
> > > > On Sun, May 05, 2013 at 02:43:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > add option to specify the configuration file
[--SNIP--]
> > > > I think we should provide a default, basic configuration file that we
> > > > install unconditionnally to the rootfs. The user will then be free to
> > > > provide a post-build script to tweak that file.
> > > 
> > > I hate post-build it's not trackable and as it's completly board specific with
> > > x serial to export I do not want a generic file
> > 
> > It's not about what *you* want, it's about what is *best* for Buildroot
> > and *all* its users, *and* is maintenable and easily reproduced (which
> > pointing to a config file outside of the buildroot tree is definitely
> > not) *and* can fit your use case (which what I suggest should).
> 
> I want to put the ser2net.conf in the tree not our of tree

OK. So, why have it configurable if it is in the tree in the first place?

> > We're speaking about a _default_ and _simple_ configuration file. Of
> > course it can not cover all use cases, but just exporting a single
> > serial port would be enough. Customisation beyond that is really part of
> > the post-build scripts.
> the problem here which port you export?
> the ttyS0? no it's mostly the default console

Of course, ttyS0 is special because it is the console. ttyS1 is a good
candidate.

> how could you even think of any generic file as the name of the port(s) and
> configuration is board specific

IIRC, I never said 'generic'; I said 'default and basic', which I agree
did not completely convey what I meant. I think this file should be seen
as a (kind of) example that would work on some boards (eg. a PC) but
would anyway need further customisation.

> This will be completly board specific

Yes, I agree that will be board specific.

However, providing a default configuration file that exports ttyS1 can
serve as a basis for the user to customise.

We can't expect everything in Buildroot to work without customisation.
For example, a webserver will always have pages to serve, so the user
will have to provide those. And provide a certificate if he wants to
serve https. And so on... The same goes for this kind of configuration:
provide a simple, basic config file that the user can further customise,
or even entirely replace.

And this kind of customisation is done with post-build scripts.

However, an alternative solution would be to ask the user what port(s)
to export, and generate the config file from ser2net.mk. If this is not
enough, then a post-build script can be used to provide a more advanced
config file.

Regards,
Yann E. MORIN.

Patch

diff --git a/package/ser2net/Config.in b/package/ser2net/Config.in
index e3b8870..3480d85 100644
--- a/package/ser2net/Config.in
+++ b/package/ser2net/Config.in
@@ -8,5 +8,14 @@  config BR2_PACKAGE_SER2NET
 
 	  http://ser2net.sourceforge.net
 
+if BR2_PACKAGE_SER2NET
+
+config BR2_PACKAGE_SER2NET_CONF
+	string "Configuration file"
+	help
+	  Configuration file.
+
+endif
+
 comment "ser2net requires a toolchain with IPV6 support"
 	depends on !BR2_INET_IPV6
diff --git a/package/ser2net/S50ser2net b/package/ser2net/S50ser2net
new file mode 100644
index 0000000..90b6af1
--- /dev/null
+++ b/package/ser2net/S50ser2net
@@ -0,0 +1,48 @@ 
+#!/bin/sh 
+
+NAME=ser2net
+DAEMON=/usr/sbin/${NAME}
+CONFIG_FILE=/etc/${NAME}.conf
+PIDFILE=/var/run/$NAME.pid
+trap "" 1
+trap "" 15
+test -f $DAEMON || exit 0
+
+start() {
+	echo -n "Starting Ser2Net: "
+	$DAEMON -c $CONFIG_FILE -P ${PIDFILE}
+	if [ $? != 0 ]; then
+		echo "FAILED"
+		exit 1
+	else
+		echo "done"
+	fi
+}
+
+stop() {
+	echo -n "Stopping Ser2Net: "
+	kill -9 `cat ${PIDFILE}`
+	echo "done"
+}
+
+case "$1" in
+    start)
+	start
+	;;
+
+    stop)
+	stop
+	;;
+
+    restart)
+	stop
+	start
+	;;
+
+    *)
+	echo "Usage: /etc/init.d/S50ser2net {start|stop|restart}"
+	exit 1
+	;;
+esac
+
+exit 0
diff --git a/package/ser2net/ser2net.mk b/package/ser2net/ser2net.mk
index 378dc06..5a2539e 100644
--- a/package/ser2net/ser2net.mk
+++ b/package/ser2net/ser2net.mk
@@ -9,4 +9,31 @@  SER2NET_SITE = http://downloads.sourceforge.net/project/ser2net/ser2net
 SER2NET_LICENSE = GPLv2+
 SER2NET_LICENSE_FILES = COPYING
 
+define SER2NET_INIT_SCRIPT_INSTALL
+	$(INSTALL) -m 0755 package/ser2net/S50ser2net $(TARGET_DIR)/etc/init.d
+endef
+
+SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_INSTALL
+
+define SER2NET_INIT_SCRIPT_UNINSTALL
+	rm -f $(TARGET_DIR)/etc/init.d/S50ser2net
+endef
+
+SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_UNINSTALL
+
+define SER2NET_CONF_INSTALL
+	if [ "$(BR2_PACKAGE_SER2NET_CONF)" != "" ] ; then \
+		$(INSTALL) -m 0644 $(BR2_PACKAGE_SER2NET_CONF) \
+			$(TARGET_DIR)/etc/ser2net.conf ; \
+	fi
+endef
+
+SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_CONF_INSTALL
+
+define SER2NET_CONFIG_UNINSTALL
+	rm -f $(TARGET_DIR)/etc/ser2net.conf
+endef
+
+SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_CONFIG_UNINSTALL
+
 $(eval $(autotools-package))