Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed serial console for syslinux #2650

Merged
merged 7 commits into from Jul 27, 2021

Conversation

DEvil0000
Copy link
Contributor

  • Type: Bug Fix
  • Impact: Normal
  • Reference to related issue (URL): kind of related Grub boot option for OUTPUT=USB (syslinux may not work with coreboot) #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
Copy link
Member

jsmeix commented Jul 7, 2021

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
Copy link
Member

jsmeix commented Jul 7, 2021

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

@DEvil0000
Copy link
Contributor Author

DEvil0000 commented Jul 7, 2021

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
Copy link
Contributor Author

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 jsmeix requested a review from a team July 14, 2021 10:11
@jsmeix jsmeix self-assigned this Jul 14, 2021
@jsmeix jsmeix added the bug The code does not do what it is meant to do label Jul 14, 2021
@jsmeix jsmeix added this to the ReaR v2.7 milestone Jul 14, 2021
@jsmeix jsmeix requested a review from gdha July 14, 2021 10:15
@jsmeix
Copy link
Member

jsmeix commented Jul 14, 2021

@DEvil0000
thank you for your additional tests!

@jsmeix
Copy link
Member

jsmeix commented Jul 14, 2021

@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
Copy link
Contributor Author

@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
Copy link
Member

jsmeix commented Jul 14, 2021

My idea of enhancing USE_SERIAL_CONSOLE as I suggested in
#2650 (comment)
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
Copy link
Member

jsmeix commented Jul 14, 2021

@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
Copy link
Contributor Author

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.

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From plain looking at the code
(I am not a serial console user)
things look good to me.

@jsmeix jsmeix requested a review from a team July 19, 2021 06:46
@jsmeix
Copy link
Member

jsmeix commented Jul 19, 2021

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

@jsmeix jsmeix merged commit 339607c into rear:master Jul 27, 2021
@jsmeix jsmeix mentioned this pull request Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The code does not do what it is meant to do fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants