#2650 PR `merged`: fixed serial console for syslinux¶

Labels: bug, fixed / solved / done

DEvil0000 opened issue at 2021-07-07 09:27:¶

Type: Bug Fix
Impact: Normal
Reference to related issue (URL): kind of related #2644
Relax-and-Recover 2.6 / 2020-06-17
syslinux 6.04 Copyright 1994-2015 H. Peter Anvin et al
extlinux 6.04 Copyright 1994-2015 H. Peter Anvin et al
How was this pull request tested?
using output=usb I created a bootable USB stick with working serial on a apu board based machine. (ex6linux)
Brief description of the changes in this pull request:
I can not find any statement in syslinux documentation if multiple serial lines are allowed. The docs just state the line must be the first one in the config file. On my apu board based machine I have two serial interfaces (and nothing else) but as soon as I have serial config lines for two different devices/ports it is not working for any of them.
Only when having one serial device in the config it is working.

So this PR does two things:

it writes only one serial line matching the configured device (when found) to the config
it also writes it for the syslinux config in case it is used without extlinux

jsmeix commented at 2021-07-07 10:43:¶

I am not at all a serial console expert so I may confuse this or that.

From plain looking at the serial console related code in ReaR
by inspecting the serial console related scripts

# find usr/share/rear/ -type f | xargs grep -li 'serial console'
usr/share/rear/output/USB/Linux-i386/300_create_extlinux.sh
usr/share/rear/conf/default.conf
usr/share/rear/rescue/GNU/Linux/400_use_serial_console.sh
usr/share/rear/skel/default/etc/scripts/system-setup.d/45-serial-console.sh
usr/share/rear/skel/default/etc/init/start-ttys.conf
usr/share/rear/lib/bootloader-functions.sh
usr/share/rear/prep/GNU/Linux/200_include_serial_console.sh

there are two different places where serial console is set up in ReaR.

Setup of serial consoles in the running ReaR recovery system
versus
setup of serial consoles for the bootloader of the ReaR recovery system

Currently both are specified via the boolean config variable USE_SERIAL_CONSOLE.

Your new SERIAL_CONSOLE_DEVICE config variable only applies
to setup of serial consoles for the bootloader of the ReaR recovery system
and there only in case of syslinux.
So that new config variable should be properly named e.g.
SERIAL_CONSOLE_DEVICE _SYSLINUX
to make its limited usage obvious and that limited usage
should be described more explicitly in default.conf
(currently 'syslinux' is mentioned but that looks more like an example case).

In general I prefer a single config variable with ternary (and more) semantics
(cf. the initial comment in disklayout.conf)
over several config variables for one same thing
if possible to implement it with reasonable effort
and if it makes things simpler to setup for the user
(we have already very many config variables).

So in this case I would prefer to drop SERIAL_CONSOLE_DEVICE
and instead enhance the current USE_SERIAL_CONSOLE like

USE_SERIAL_CONSOLE is empty: same behaviour as now
USE_SERIAL_CONSOLE has a true or false value: same behaviour as now
USE_SERIAL_CONSOLE is a string of possible character device nodes: serial consoles are set up only for those device nodes that exist in USE_SERIAL_CONSOLE

So e.g. USE_SERIAL_CONSOLE="true /dev/ttyS0 /dev/sda"
will set up a serial console only for /dev/ttyS0 if /dev/ttyS0 exists
(/dev/sda is no character device node and 'true' is no device node)
while USE_SERIAL_CONSOLE="/dev/ttyS2 /dev/ttyS123"
will set up serial consoles for /dev/ttyS2 and /dev/ttyS123 if exists.

jsmeix commented at 2021-07-07 12:21:¶

FYI: I will be "offline" until next Monday.

DEvil0000 commented at 2021-07-07 14:19:¶

The fix is absolutely syslinux & extlinux config and not related to other serial config. It may be syslinux version, hardware and bios dependent but thats something I can't tell.

Those scripts do take care about different serial related config (kernel parameter and getty config):

usr/share/rear/rescue/GNU/Linux/400_use_serial_console.sh
usr/share/rear/skel/default/etc/scripts/system-setup.d/45-serial-console.sh
usr/share/rear/skel/default/etc/init/start-ttys.conf
usr/share/rear/prep/GNU/Linux/200_include_serial_console.sh

The script usr/share/rear/lib/bootloader-functions.sh however looks interesting. The syslinux part looks mostly identical and has the same issue. I guess this is the more modern bootloader function set that should have been used by the usb scripts instead of doing basically the same thing with duplicated code.
My intention was more to fix my specific issue and not rewrite the whole format/output usb scripts but I will have a look when I find the time for it.

I agree that a string containing devices for USE_SERIAL_CONSOLE would make sense instead of a new variable but that would mean chaning a lot of scripts with usecases I don't know and can't test. So changing the USE_SERIAL_CONSOLE allowed values brings a way bigger risk of breaking things which I did not want to take.
The name SERIAL_CONSOLE_DEVICE_SYSLINUX is fine for me even when it actually also changes extlinux config far as I understand.

DEvil0000 commented at 2021-07-12 10:38:¶

strangely multiple serial config lines work on apu2 board versions. So I will change the PR to keep the old behavior when the variable is not set.

jsmeix commented at 2021-07-14 10:15:¶

@DEvil0000
thank you for your additional tests!

jsmeix commented at 2021-07-14 10:21:¶

@gdha @pcahyna
I would like to know your opinion about a new config variable
SERIAL_CONSOLE_DEVICE_SYSLINUX
introduced by this pull request
versus some generic cleanup and enhancement of the existing
USE_SERIAL_CONSOLE
config variable.

@DEvil0000
if there is some general agreement to better implement it
via some generic cleanup and enhancement of the existing
USE_SERIAL_CONSOLE
I would try to do that on my own via a separated pull request
so you could have a look and try out if that also works for you.

DEvil0000 commented at 2021-07-14 10:31:¶

@jsmeix In case you decide to change how USE_SERIAL_CONSOLE works it may be best to merge the PR first and handle it then like the rest of the code.

jsmeix commented at 2021-07-14 10:31:¶

My idea of enhancing USE_SERIAL_CONSOLE as I suggested in
https://github.com/rear/rear/pull/2650#issuecomment-875498171
is that e.g. with

USE_SERIAL_CONSOLE="/dev/ttyS0"

both the serial console for the bootloader
and the serial console for the ReaR recovery system
will be only at "/dev/ttyS0" (and nowhere else).

My reasoning behind is that I assume when a user specifies e.g.

USE_SERIAL_CONSOLE="/dev/ttyS0"

then he has a serial console only at /dev/ttyS0 and nowhere else.

Or in other words:
I ssume it will not happen in practice that a user has
e.g. two serial consoles at /dev/ttyS0 and /dev/ttyS1
but for the bootloader (e.g. in case of syslinux)
only one serial consoles at /dev/ttyS0 can be used
but for the ReaR recovery system he wants to use
both serial consoles at /dev/ttyS0 and /dev/ttyS1

If that assumption is wrong my proposal
of enhancing USE_SERIAL_CONSOLE cannot work
and a separated SERIAL_CONSOLE_DEVICE_SYSLINUX
must be used.

jsmeix commented at 2021-07-14 10:33:¶

@DEvil0000
from my point of view I could merge your pull request just now
if you removed the SERIAL_CONSOLE_DEVICE_SYSLINUX
part in usr/share/rear/conf/default.conf so it is not yet "officially" documented
or you enhance the comment there that it may change in near future
in arbitrary ways.

DEvil0000 commented at 2021-07-16 16:43:¶

added the not about a near future change of USE_SERIAL_CONSOLE and SERIAL_CONSOLE_DEVICE_SYSLINUX to the comment in the file as requested. So this one should be okay for merge now.

jsmeix commented at 2021-07-19 06:46:¶

@rear/contributors
if there are no objections I would like to merge it tomorrow afternoon.

[Export of Github issue for rear/rear.]

#2650 PR merged: fixed serial console for syslinux¶