Merge lp:~clint-fewbar/ubuntu/natty/upstart/add-serial-console into lp:ubuntu/natty/upstart

[Clint Byrum]

resubmitting merge proposal as the last one was stacked improperly.

adds serial console that only starts when console= is passed on kernel commandline, and which pulls those arguments out to set serial parameters.

[lp:~clint-fewbar/ubuntu/natty/upstart/add-serial-console]

1293. By Clint Byrum on 2011-01-13

adding missed new conf file

[Scott James Remnant (scott)]

Hi Clint,

This merge request only consists of a changelog entry?

Do you think it reasonable that we parse the console= line in Upstart
itself, and make the value of that available to jobs that want it.
What kind of consoles do we want to support? Do we want to support
multiple console= values? How does the kernel interpret this field
already?

I think this is a discussion you should start on the upstart mailing
list, because it seems like something we need "smart" handling for
built-in.

Scott

[Clint Byrum (clint-fewbar)]

On Fri, 2011-01-14 at 10:04 +0000, Scott James Remnant wrote:
> Hi Clint,
>
> This merge request only consists of a changelog entry?
>

I had forgotten to bzr add. The file is now listed in the MP.

>
> Do you think it reasonable that we parse the console= line in Upstart
> itself, and make the value of that available to jobs that want it.
> What kind of consoles do we want to support? Do we want to support
> multiple console= values? How does the kernel interpret this field
> already?
>

I love that idea.

> I think this is a discussion you should start on the upstart mailing
> list, because it seems like something we need "smart" handling for
> built-in.
>

Sounds good, I'll take it up there.

[James Hunt (jamesodhunt)]

Hi Guys,

From reviewing the following...

http://www.kernel.org/doc/Documentation/kernel-parameters.txt
http://www.kernel.org/doc/Documentation/serial-console.txt

...and playing with my new USB setup, my comments on "debian/conf/ttyS.conf" are below:

SPECIFICS
=========

- Line 43: I think we should be checking for "console=tty*" to avoid matching when
  user specifies "console=uart...".

- Line 45: Might be simpler to write this as:

  [ ! -z `echo $arg | egrep "tty([0-9]+)"` ] && exit 0

- Line 52: Again, simpler to write as (same logic applies for lines 62,63,65):

  [ -f /etc/init/$PORT.conf ] && exit 0

- Line 54: Assumption that parity always specified as 'n'.

- Line 63: If flow control specified, BITS could be set to something like "8r" which causes
  this test to fail.

- Line 68: Missing ";;" denoting end of 'console=*)' case statement (appears to be non-fatal).

GENERICS
========

- Is it a reasonable assumption that we only consider a single serial console device?

- As mentioned in comment on Line 54 above, we are not parsing the options ("bbbbpnf")
  correctly. If I boot with the following, the script will fail:

    console=ttyUSB0,115200n8r

- I wonder if we should name the conf file "tty-serial.conf" rather than "ttyS.conf"
  to recognize that users might not be using serial device /dev/ttyS* but also /dev/ttyUSB*?

review: Needs Fixing

[Colin Watson (cjwatson)]

On Wed, Jan 26, 2011 at 10:06:14AM -0000, James Hunt wrote:
> - Line 45: Might be simpler to write this as:
>
> [ ! -z `echo $arg | egrep "tty([0-9]+)"` ] && exit 0

Drive-by shell style nitpicking - I'd say this instead:

  echo "$arg" | egrep -q 'tty([0-9]+)' && exit 0

(i.e. use exit status rather than testing contents.)

> - I wonder if we should name the conf file "tty-serial.conf" rather than "ttyS.conf"
> to recognize that users might not be using serial device /dev/ttyS* but also /dev/ttyUSB*?

One thing to bear in mind with all of this is that debian-installer
already dynamically creates /etc/init/tty*.conf when you install on a
serial console, and the file name it uses comes straight from the device
name. Using /etc/init/ttyS.conf would certainly conflict with that in
an interesting way, which indeed suggests to me that something like
tty-serial.conf would be better, but we also ought to think about how
the whole thing plays together; maybe the installer code should be
obsoleted, but there's a fair bit of intelligence in there particularly
for some of the more obscure systems. In particular there are some
systems where the installer automatically sets up a serial console even
if you don't say console=.

Have a look at finish-install.d/90console in lp:finish-install.

[Oliver Grawert (ogra)]

>
> - Line 45: Might be simpler to write this as:
>
> [ ! -z `echo $arg | egrep "tty([0-9]+)"` ] && exit 0
>
this will definitely not work for things like ttyAMR0 or ttyO2 (as imx5x or omap nowadays use them for example)

Unmerged revisions

1293. By Clint Byrum on 2011-01-13

adding missed new conf file

1292. By Clint Byrum on 2011-01-13

debian/conf/ttyS.conf: Run getty on the serial console. (LP: #702574)

1291. By Scott James Remnant (Canonical) on 2011-01-04

* debian/rules: make sure apparmor-profile-load is executable.
* debian/apparmor-profile-load: common AppArmor profile loading helper
  which can be used by any upstart services, regardless of the state
  of AppArmor (LP: #692801).

1290. By James Hunt on 2010-12-14

Begin new release.

1289. By James Hunt on 2010-12-14

releasing version 0.6.7-1

1288. By James Hunt on 2010-12-14

Added myself as a maintainer.

1287. By James Hunt on 2010-12-14

* New upstream release:
  - Added manual stanza.
  - Added debug stanza.
  - Added start_on, stop_on and emits properties.
  - Added GoalChanged, StateChanged and Failed signals.
  - Documentation updates.

1286. By James Hunt on 2010-12-14

New upstream release.

=== modified file 'debian/changelog'
--- debian/changelog	2011-01-04 21:14:44 +0000
+++ debian/changelog	2011-01-13 21:57:08 +0000
@@ -1,3 +1,9 @@
+upstart (0.6.7-4) natty; urgency=low
+
+  * debian/conf/ttyS.conf: Run getty on the serial console. (LP: #702574)
+
+ -- Clint Byrum <[e-mail]>  Thu, 13 Jan 2011 12:39:51 -0800
+
 upstart (0.6.7-3) natty; urgency=low
 
   * debian/rules: make sure apparmor-profile-load is executable.
 
=== added file 'debian/conf/ttyS.conf'
--- debian/conf/ttyS.conf	1970-01-01 00:00:00 +0000
+++ debian/conf/ttyS.conf	2011-01-13 21:57:08 +0000
@@ -0,0 +1,53 @@
+# - ttyS
+#
+
+description "This service maintains a getty on serial port given as 'console' kernel argument."
+author "Oliver Grawert <[e-mail]> and Clint Byrum <[e-mail]>"
+
+#
+# Last 'console' argument is used
+#
+
+start on runlevel [23]
+stop on runlevel [!23]
+
+respawn
+
+script
+    # check for console entry
+    if [ -z "`grep console /proc/cmdline`" ]; then
+        exit 0
+    fi
+
+    for arg in $(cat /proc/cmdline)
+    do
+    case $arg in
+        console=*)
+        # if VT then exit
+        if [ ! -z `echo $arg | egrep "tty([0-9]+)"` ];then exit 0;fi
+
+        IFS=','
+        set -- ${arg#console=}
+        PORT=${1#/dev/}
+
+        # check for service which do something on this port
+        if [ -f /etc/init/$PORT.conf ];then exit 0;fi
+
+        IFS='n'
+        set -- $2
+        SPEED=$1
+        BITS=$2
+
+        GETTY_ARGS=""
+
+        # 8bit serial is default
+        if [ -z $BITS ]; then BITS=8;fi
+        if [ 8 -eq $BITS ]; then GETTY_ARGS="$GETTY_ARGS -8 ";fi
+
+        if [ -z $SPEED ]; then SPEED='115200,57600,38400,19200,9600';fi
+
+        GETTY_ARGS="$GETTY_ARGS $SPEED $PORT"
+        exec /sbin/getty $GETTY_ARGS
+    esac
+    done
+end script