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

Proposed by Clint Byrum
Status: Needs review
Proposed branch: lp:~clint-fewbar/ubuntu/natty/upstart/add-serial-console
Merge into: lp:ubuntu/natty/upstart
Diff against target: 71 lines (+59/-0)
2 files modified
debian/changelog (+6/-0)
debian/conf/ttyS.conf (+53/-0)
To merge this branch: bzr merge lp:~clint-fewbar/ubuntu/natty/upstart/add-serial-console
Reviewer Review Type Date Requested Status
James Hunt (community) Needs Fixing
Scott James Remnant Pending
Review via email: mp+46191@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
1293. By Clint Byrum

adding missed new conf file

Revision history for this message
Scott James Remnant (scott) wrote :

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

Revision history for this message
Clint Byrum (clint-fewbar) wrote :

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.

Revision history for this message
James Hunt (jamesodhunt) wrote :

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
Revision history for this message
Colin Watson (cjwatson) wrote :

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.

Revision history for this message
Oliver Grawert (ogra) wrote :

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

adding missed new conf file

1292. By Clint Byrum

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

1291. By Scott James Remnant (Canonical)

* 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

Begin new release.

1289. By James Hunt

releasing version 0.6.7-1

1288. By James Hunt

Added myself as a maintainer.

1287. By James Hunt

* 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

New upstream release.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2011-01-04 21:14:44 +0000
3+++ debian/changelog 2011-01-13 21:57:08 +0000
4@@ -1,3 +1,9 @@
5+upstart (0.6.7-4) natty; urgency=low
6+
7+ * debian/conf/ttyS.conf: Run getty on the serial console. (LP: #702574)
8+
9+ -- Clint Byrum <clint@ubuntu.com> Thu, 13 Jan 2011 12:39:51 -0800
10+
11 upstart (0.6.7-3) natty; urgency=low
12
13 * debian/rules: make sure apparmor-profile-load is executable.
14
15=== added file 'debian/conf/ttyS.conf'
16--- debian/conf/ttyS.conf 1970-01-01 00:00:00 +0000
17+++ debian/conf/ttyS.conf 2011-01-13 21:57:08 +0000
18@@ -0,0 +1,53 @@
19+# - ttyS
20+#
21+
22+description "This service maintains a getty on serial port given as 'console' kernel argument."
23+author "Oliver Grawert <ogra@ubuntu.com> and Clint Byrum <clint@ubuntu.com>"
24+
25+#
26+# Last 'console' argument is used
27+#
28+
29+start on runlevel [23]
30+stop on runlevel [!23]
31+
32+respawn
33+
34+script
35+ # check for console entry
36+ if [ -z "`grep console /proc/cmdline`" ]; then
37+ exit 0
38+ fi
39+
40+ for arg in $(cat /proc/cmdline)
41+ do
42+ case $arg in
43+ console=*)
44+ # if VT then exit
45+ if [ ! -z `echo $arg | egrep "tty([0-9]+)"` ];then exit 0;fi
46+
47+ IFS=','
48+ set -- ${arg#console=}
49+ PORT=${1#/dev/}
50+
51+ # check for service which do something on this port
52+ if [ -f /etc/init/$PORT.conf ];then exit 0;fi
53+
54+ IFS='n'
55+ set -- $2
56+ SPEED=$1
57+ BITS=$2
58+
59+ GETTY_ARGS=""
60+
61+ # 8bit serial is default
62+ if [ -z $BITS ]; then BITS=8;fi
63+ if [ 8 -eq $BITS ]; then GETTY_ARGS="$GETTY_ARGS -8 ";fi
64+
65+ if [ -z $SPEED ]; then SPEED='115200,57600,38400,19200,9600';fi
66+
67+ GETTY_ARGS="$GETTY_ARGS $SPEED $PORT"
68+ exec /sbin/getty $GETTY_ARGS
69+ esac
70+ done
71+end script

Subscribers

People subscribed via source and target branches

to all changes: