Code review comment for lp:~clint-fewbar/ubuntu/natty/upstart/add-serial-console

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

« Back to merge proposal