From 99b43becf8104c5ce00ba99962202301ddaf2d72 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Thu, 8 Sep 2022 01:00:58 -0400 Subject: [PATCH 1/8] install: absorb bls_entry_options_write_platform into caller It was originally factored out for unit testing, but these days we just use KargsEditor which has its own tests. Simplify by using KargsEditor directly, with a separate initial probe to ensure we can find the ignition.platform.id. --- src/install.rs | 106 ++++++++++--------------------------------------- 1 file changed, 21 insertions(+), 85 deletions(-) diff --git a/src/install.rs b/src/install.rs index 2ee95631c..80971c479 100644 --- a/src/install.rs +++ b/src/install.rs @@ -540,6 +540,21 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> { if platform == "metal" { return Ok(()); } + eprintln!("Setting platform to {}", platform); + + // Early check that we can find the expected platform ID. We assume + // that we will only install from metal images and that the bootloader + // configs will always set ignition.platform.id. + visit_bls_entry_options(mountpoint, |orig_options: &str| { + let new_options = KargsEditor::new() + .replace(&[format!("ignition.platform.id=metal={}", platform)]) + .apply_to(orig_options) + .context("searching for platform ID")?; + if orig_options == new_options { + bail!("couldn't locate platform ID"); + } + Ok(None) + })?; // read platforms table let (spec, metal_spec) = match fs::read_to_string(mountpoint.join("coreos/platforms.json")) { @@ -558,14 +573,13 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> { }; // set kargs, removing any metal-specific ones - eprintln!("Setting platform to {}", platform); visit_bls_entry_options(mountpoint, |orig_options: &str| { - bls_entry_options_write_platform( - orig_options, - platform, - &spec.kernel_arguments, - &metal_spec.kernel_arguments, - ) + KargsEditor::new() + .replace(&[format!("ignition.platform.id=metal={}", platform)]) + .append(&spec.kernel_arguments) + .delete(&metal_spec.kernel_arguments) + .maybe_apply_to(orig_options) + .context("setting platform kernel arguments") })?; // set grub commands @@ -579,34 +593,6 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> { Ok(()) } -/// To be used with `visit_bls_entry_options()`. Modifies the BLS config, -/// changing the `ignition.platform.id` and then appending/deleting any -/// specified kargs. This assumes that we will only install from metal -/// images and that the bootloader configs will always set -/// ignition.platform.id. Fail if those assumptions change. This is -/// deliberately simplistic. -fn bls_entry_options_write_platform( - orig_options: &str, - platform: &str, - append_kargs: &[String], - delete_kargs: &[String], -) -> Result> { - let new_options = KargsEditor::new() - .replace(&[format!("ignition.platform.id=metal={}", platform)]) - .apply_to(orig_options) - .context("updating platform ID")?; - if orig_options == new_options { - bail!("Couldn't locate platform ID"); - } - Ok(Some( - KargsEditor::new() - .append(append_kargs) - .delete(delete_kargs) - .apply_to(&new_options) - .context("adding platform-specific kernel arguments")?, - )) -} - /// Rewrite the grub.cfg CONSOLE-SETTINGS block to use the specified GRUB /// commands, and return the result. fn update_grub_cfg_console_settings(grub_cfg: &str, commands: &[String]) -> Result { @@ -778,56 +764,6 @@ mod tests { } } - #[test] - fn test_platform_id() { - let orig_content = "ignition.platform.id=metal foo bar"; - let new_content = bls_entry_options_write_platform( - orig_content, - "openstack", - &vec!["baz".to_string(), "blah".to_string()], - &[], - ) - .unwrap(); - assert_eq!( - new_content.unwrap(), - "ignition.platform.id=openstack foo bar baz blah" - ); - - let orig_content = "foo ignition.platform.id=metal bar"; - let new_content = bls_entry_options_write_platform( - orig_content, - "openstack", - &vec!["baz".to_string(), "blah".to_string()], - &vec!["foo".to_string()], - ) - .unwrap(); - assert_eq!( - new_content.unwrap(), - "ignition.platform.id=openstack bar baz blah" - ); - - let orig_content = "foo bar ignition.platform.id=metal"; - let new_content = bls_entry_options_write_platform( - orig_content, - "openstack", - &vec!["baz".to_string(), "blah".to_string()], - &[], - ) - .unwrap(); - assert_eq!( - new_content.unwrap(), - "foo bar ignition.platform.id=openstack baz blah" - ); - - let orig_content = "foo bar ignition.platform.id=metal"; - let new_content = - bls_entry_options_write_platform(orig_content, "openstack", &[], &[]).unwrap(); - assert_eq!( - new_content.unwrap(), - "foo bar ignition.platform.id=openstack" - ); - } - #[test] fn test_update_grub_cfg() { let base_cfgs = vec![ From 4c57a1f24b23e3c8a8a1b31212555891ab89a4fa Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Thu, 8 Sep 2022 09:21:56 -0400 Subject: [PATCH 2/8] install: split console configuration into separate function This has us rewriting the BLS config twice if `--platform` is specified, but that option is non-performance-critical and also an edge case, so let's aim for legibility. Preparatory for next commit. --- src/install.rs | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/install.rs b/src/install.rs index 80971c479..faa2e321d 100644 --- a/src/install.rs +++ b/src/install.rs @@ -404,6 +404,7 @@ fn write_disk( } if let Some(platform) = config.platform.as_ref() { write_platform(mount.mountpoint(), platform).context("writing platform ID")?; + write_console(mount.mountpoint(), platform).context("configuring console")?; } if let Some(firstboot_args) = config.firstboot_args.as_ref() { write_firstboot_kargs(mount.mountpoint(), firstboot_args) @@ -532,8 +533,7 @@ struct PlatformSpec { kernel_arguments: Vec, } -/// Override the platform ID. Add any kernel arguments and grub.cfg -/// directives specified for this platform in platforms.json. +/// Override the platform ID. fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> { // early return if setting the platform to the default value, since // otherwise we'll think we failed to set it @@ -542,19 +542,27 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> { } eprintln!("Setting platform to {}", platform); - // Early check that we can find the expected platform ID. We assume - // that we will only install from metal images and that the bootloader - // configs will always set ignition.platform.id. + // We assume that we will only install from metal images and that the + // bootloader configs will always set ignition.platform.id. visit_bls_entry_options(mountpoint, |orig_options: &str| { let new_options = KargsEditor::new() .replace(&[format!("ignition.platform.id=metal={}", platform)]) .apply_to(orig_options) - .context("searching for platform ID")?; + .context("setting platform ID argument")?; if orig_options == new_options { bail!("couldn't locate platform ID"); } - Ok(None) + Ok(Some(new_options)) })?; + Ok(()) +} + +/// Configure console kernel arguments and GRUB commands. +fn write_console(mountpoint: &Path, platform: &str) -> Result<()> { + // anything to do? + if platform == "metal" { + return Ok(()); + } // read platforms table let (spec, metal_spec) = match fs::read_to_string(mountpoint.join("coreos/platforms.json")) { @@ -575,7 +583,6 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> { // set kargs, removing any metal-specific ones visit_bls_entry_options(mountpoint, |orig_options: &str| { KargsEditor::new() - .replace(&[format!("ignition.platform.id=metal={}", platform)]) .append(&spec.kernel_arguments) .delete(&metal_spec.kernel_arguments) .maybe_apply_to(orig_options) From dced43b87c3e3b1e6e27de6e812aa93efdc1a7bf Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Thu, 8 Sep 2022 03:55:25 -0400 Subject: [PATCH 3/8] install: add --console to configure GRUB and kernel console Once CoreOS stops enabling the serial console by default, users will need a way to enable it themselves. This really only affects bare metal, since other platforms have a consistent console configuration, so coreos-installer is a good place to handle it. Users can already pass console= kargs, but we need a way to specify GRUB commands, ideally without having to actually use GRUB syntax. Add a --console option that accepts kernel console= syntax, since that's already familiar. Parse the option and convert it to both a console karg and GRUB commands, using the existing mechanism for GRUB command substitution. If --platform is also specified, have --console override that platform's default console settings. If --console is specified more than once, aggregate the console settings, following the multiple-console semantics for GRUB (use both consoles) and the kernel (use all consoles, with the last console primary). There are serial console features only supported by GRUB (stop bits) or by the kernel (RTS/CTS flow control). Support the common subset and reject the rest. Also, round-trip console kargs to a less-compact but semantically equivalent format, since explicit is good and it saves some code complexity. Document this as "bootloader console" rather than "GRUB console" to allow for future bootloaders other than GRUB. --- data/example-config.yaml | 2 + docs/cmd/install.md | 6 + docs/customizing-install.md | 2 + docs/release-notes.md | 1 + man/coreos-installer-install.8 | 7 +- src/cmdline/console.rs | 260 +++++++++++++++++++++++++++++++++ src/cmdline/install.rs | 22 +++ src/cmdline/mod.rs | 2 + src/install.rs | 70 ++++++--- 9 files changed, 351 insertions(+), 21 deletions(-) create mode 100644 src/cmdline/console.rs diff --git a/data/example-config.yaml b/data/example-config.yaml index fcf7c1acf..e0a8aa299 100644 --- a/data/example-config.yaml +++ b/data/example-config.yaml @@ -17,6 +17,8 @@ ignition-hash: digest architecture: name # Override the Ignition platform ID platform: name +# Kernel and bootloader console +console: [spec, spec] # Append default kernel arguments append-karg: [arg, arg] # Delete default kernel arguments diff --git a/docs/cmd/install.md b/docs/cmd/install.md index 150afe84a..155b29d46 100644 --- a/docs/cmd/install.md +++ b/docs/cmd/install.md @@ -69,6 +69,12 @@ OPTIONS: Install a system that will run on the specified cloud or virtualization platform, such as "vmware". + --console + Kernel and bootloader console + + Set the kernel and bootloader console, using the same syntax as the parameter to + the "console=" kernel argument. + --append-karg Append default kernel arg diff --git a/docs/customizing-install.md b/docs/customizing-install.md index 9612b4885..d2cd4f99f 100644 --- a/docs/customizing-install.md +++ b/docs/customizing-install.md @@ -130,6 +130,8 @@ ignition-hash: digest architecture: name # Override the Ignition platform ID platform: name +# Kernel and bootloader console +console: [spec, spec] # Append default kernel arguments append-karg: [arg, arg] # Delete default kernel arguments diff --git a/docs/release-notes.md b/docs/release-notes.md index 3aeda7ab3..fdc3e0e92 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -8,6 +8,7 @@ nav_order: 8 Major changes: +- install: Add `--console` for configuring kernel and bootloader console - Support reading initrd images compressed with zstd - Add Fedora 38 signing key; drop Fedora 35 signing key diff --git a/man/coreos-installer-install.8 b/man/coreos-installer-install.8 index e90e702c1..deb390448 100644 --- a/man/coreos-installer-install.8 +++ b/man/coreos-installer-install.8 @@ -4,7 +4,7 @@ .SH NAME coreos\-installer\-install \- Install Fedora CoreOS or RHEL CoreOS .SH SYNOPSIS -\fBcoreos\-installer\-install\fR [\fB\-h\fR|\fB\-\-help\fR] [\fB\-V\fR|\fB\-\-version\fR] [\fB\-c\fR|\fB\-\-config\-file\fR] [\fB\-s\fR|\fB\-\-stream\fR] [\fB\-u\fR|\fB\-\-image\-url\fR] [\fB\-f\fR|\fB\-\-image\-file\fR] [\fB\-i\fR|\fB\-\-ignition\-file\fR] [\fB\-I\fR|\fB\-\-ignition\-url\fR] [\fB\-\-ignition\-hash\fR] [\fB\-a\fR|\fB\-\-architecture\fR] [\fB\-p\fR|\fB\-\-platform\fR] [\fB\-\-append\-karg\fR] [\fB\-\-delete\-karg\fR] [\fB\-n\fR|\fB\-\-copy\-network\fR] [\fB\-\-network\-dir\fR] [\fB\-\-save\-partlabel\fR] [\fB\-\-save\-partindex\fR] [\fB\-\-offline\fR] [\fB\-\-insecure\fR] [\fB\-\-insecure\-ignition\fR] [\fB\-\-stream\-base\-url\fR] [\fB\-\-preserve\-on\-error\fR] [\fB\-\-fetch\-retries\fR] [\fIDEST_DEVICE\fR] +\fBcoreos\-installer\-install\fR [\fB\-h\fR|\fB\-\-help\fR] [\fB\-V\fR|\fB\-\-version\fR] [\fB\-c\fR|\fB\-\-config\-file\fR] [\fB\-s\fR|\fB\-\-stream\fR] [\fB\-u\fR|\fB\-\-image\-url\fR] [\fB\-f\fR|\fB\-\-image\-file\fR] [\fB\-i\fR|\fB\-\-ignition\-file\fR] [\fB\-I\fR|\fB\-\-ignition\-url\fR] [\fB\-\-ignition\-hash\fR] [\fB\-a\fR|\fB\-\-architecture\fR] [\fB\-p\fR|\fB\-\-platform\fR] [\fB\-\-console\fR] [\fB\-\-append\-karg\fR] [\fB\-\-delete\-karg\fR] [\fB\-n\fR|\fB\-\-copy\-network\fR] [\fB\-\-network\-dir\fR] [\fB\-\-save\-partlabel\fR] [\fB\-\-save\-partindex\fR] [\fB\-\-offline\fR] [\fB\-\-insecure\fR] [\fB\-\-insecure\-ignition\fR] [\fB\-\-stream\-base\-url\fR] [\fB\-\-preserve\-on\-error\fR] [\fB\-\-fetch\-retries\fR] [\fIDEST_DEVICE\fR] .SH DESCRIPTION Install Fedora CoreOS or RHEL CoreOS .SH OPTIONS @@ -56,6 +56,11 @@ Override the Ignition platform ID Install a system that will run on the specified cloud or virtualization platform, such as "vmware". .TP +\fB\-\-console\fR=\fIspec\fR +Kernel and bootloader console + +Set the kernel and bootloader console, using the same syntax as the parameter to the "console=" kernel argument. +.TP \fB\-\-append\-karg\fR=\fIarg\fR Append default kernel arg diff --git a/src/cmdline/console.rs b/src/cmdline/console.rs new file mode 100644 index 000000000..72eae361e --- /dev/null +++ b/src/cmdline/console.rs @@ -0,0 +1,260 @@ +// Copyright 2022 Red Hat, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Helper types for console argument. + +use anyhow::{bail, Context, Error, Result}; +use lazy_static::lazy_static; +use regex::Regex; +use serde_with::{DeserializeFromStr, SerializeDisplay}; +use std::fmt; +use std::str::FromStr; + +const KARG_PREFIX: &str = "console="; + +#[derive(Clone, Debug, DeserializeFromStr, SerializeDisplay, PartialEq, Eq)] +pub enum Console { + Graphical(GraphicalConsole), + Serial(SerialConsole), +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct GraphicalConsole { + device: String, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct SerialConsole { + prefix: String, + port: u8, + speed: u32, + data_bits: u8, + parity: Parity, + // Linux console doesn't support stop bits + // GRUB doesn't support RTS/CTS flow control +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +enum Parity { + None, + Odd, + Even, +} + +impl Parity { + fn for_grub(&self) -> &'static str { + match self { + Self::None => "no", + Self::Odd => "odd", + Self::Even => "even", + } + } + + fn for_karg(&self) -> &'static str { + match self { + Self::None => "n", + Self::Odd => "o", + Self::Even => "e", + } + } +} + +impl Console { + pub fn grub_terminal(&self) -> &'static str { + match self { + Self::Graphical(_) => "console", + Self::Serial(_) => "serial", + } + } + + pub fn grub_command(&self) -> Option { + match self { + Self::Graphical(_) => None, + Self::Serial(c) => Some(format!( + "serial --unit={} --speed={} --word={} --parity={}", + c.port, + c.speed, + c.data_bits, + c.parity.for_grub() + )), + } + } + + pub fn karg(&self) -> String { + format!("{KARG_PREFIX}{}", self) + } +} + +impl FromStr for Console { + type Err = Error; + + fn from_str(s: &str) -> Result { + // help the user with possible misunderstandings + for prefix in [KARG_PREFIX, "/dev/"] { + if s.starts_with(prefix) { + bail!(r#"spec should not start with "{prefix}""#); + } + } + + // first, parse serial console parameters + lazy_static! { + static ref SERIAL_REGEX: Regex = Regex::new("^(?PttyS|ttyAMA)(?P[0-9]+)(?:,(?P[0-9]+)(?:(?Pn|o|e)(?P[5-8])?)?)?$").expect("compiling console regex"); + } + if let Some(c) = SERIAL_REGEX.captures(s) { + return Ok(Console::Serial(SerialConsole { + prefix: c + .name("prefix") + .expect("prefix is mandatory") + .as_str() + .to_string(), + port: c + .name("port") + .expect("port is mandatory") + .as_str() + .parse() + .context("couldn't parse port")?, + speed: c + .name("speed") + .map(|v| v.as_str().parse().context("couldn't parse speed")) + .unwrap_or(Ok(9600))?, + data_bits: c + .name("data_bits") + .map(|v| v.as_str().parse().expect("unexpected data bits")) + .unwrap_or(8), + parity: match c.name("parity").map(|v| v.as_str()) { + // default + None => Parity::None, + Some("n") => Parity::None, + Some("e") => Parity::Even, + Some("o") => Parity::Odd, + _ => unreachable!(), + }, + })); + } + + // then try hardcoded strings for graphical consoles + match s { + "tty0" | "hvc0" | "ttysclp0" => Ok(Console::Graphical(GraphicalConsole { + device: s.to_string(), + })), + _ => bail!("invalid or unsupported console argument"), + } + } +} + +impl fmt::Display for Console { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Graphical(c) => write!(f, "{}", c.device), + Self::Serial(c) => write!( + f, + "{}{},{}{}{}", + c.prefix, + c.port, + c.speed, + c.parity.for_karg(), + c.data_bits + ), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn valid_console_args() { + let cases = vec![ + ("tty0", "console=tty0", "console", None), + ("hvc0", "console=hvc0", "console", None), + ("ttysclp0", "console=ttysclp0", "console", None), + ( + "ttyS1", + "console=ttyS1,9600n8", + "serial", + Some("serial --unit=1 --speed=9600 --word=8 --parity=no"), + ), + ( + "ttyAMA1", + "console=ttyAMA1,9600n8", + "serial", + Some("serial --unit=1 --speed=9600 --word=8 --parity=no"), + ), + ( + "ttyS1,1234567e5", + "console=ttyS1,1234567e5", + "serial", + Some("serial --unit=1 --speed=1234567 --word=5 --parity=even"), + ), + ( + "ttyS2,5o", + "console=ttyS2,5o8", + "serial", + Some("serial --unit=2 --speed=5 --word=8 --parity=odd"), + ), + ( + "ttyS3,17", + "console=ttyS3,17n8", + "serial", + Some("serial --unit=3 --speed=17 --word=8 --parity=no"), + ), + ]; + for (input, karg, grub_terminal, grub_command) in cases { + let console = Console::from_str(input).unwrap(); + assert_eq!( + console.grub_terminal(), + grub_terminal, + "GRUB terminal for {}", + input + ); + assert_eq!( + console.grub_command().as_deref(), + grub_command, + "GRUB command for {}", + input + ); + assert_eq!(console.karg(), karg, "karg for {}", input); + } + } + + #[test] + fn invalid_console_args() { + let cases = vec![ + "foo", + "/dev/tty0", + "/dev/ttyS0", + "console=tty0", + "console=ttyS0", + "ztty0", + "zttyS0", + "tty0z", + "ttyS0z", + "tty1", + "hvc1", + "ttysclp1", + "ttyS0,", + "ttyS0,z", + "ttyS0,115200p8", + "ttyS0,115200n4", + "ttyS0,115200n8r", + "ttyB0", + "ttyS9999999999999999999", + "ttyS0,999999999999999999999", + ]; + for input in cases { + Console::from_str(input).unwrap_err(); + } + } +} diff --git a/src/cmdline/install.rs b/src/cmdline/install.rs index b49e95f95..3da8abeaa 100644 --- a/src/cmdline/install.rs +++ b/src/cmdline/install.rs @@ -25,6 +25,7 @@ use std::fs::OpenOptions; use crate::io::IgnitionHash; +use super::console::Console; use super::serializer; use super::types::*; use super::Cmd; @@ -123,6 +124,13 @@ pub struct InstallConfig { /// virtualization platform, such as "vmware". #[clap(short, long, value_name = "name")] pub platform: Option, + /// Kernel and bootloader console + /// + /// Set the kernel and bootloader console, using the same syntax as the + /// parameter to the "console=" kernel argument. + #[serde(skip_serializing_if = "is_default")] + #[clap(long, value_name = "spec")] + pub console: Vec, /// Additional kernel args for the first boot // This used to be for configuring networking from the cmdline, but it has // been obsoleted by the nicer `--copy-network` approach. We still need it @@ -305,6 +313,10 @@ mod test { ), architecture: DefaultedString::::from_str("h").unwrap(), platform: Some("i".into()), + console: vec![ + Console::from_str("ttyS0").unwrap(), + Console::from_str("ttyS1,115200n8").unwrap(), + ], // skipped firstboot_args: Some("j".into()), append_karg: vec!["k".into(), "l".into()], @@ -338,6 +350,11 @@ mod test { "h", "--platform", "i", + "--console", + // we round-trip to an equivalent but not identical value + "ttyS0,9600n8", + "--console", + "ttyS1,115200n8", "--append-karg", "k", "--append-karg", @@ -382,6 +399,7 @@ ignition-url: http://example.com/g ignition-hash: sha256-e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 architecture: h platform: i +console: [ttyS0, "ttyS1,115200n8"] append-karg: [k, l] delete-karg: [m, n] copy-network: true @@ -418,6 +436,10 @@ dest-device: u ), architecture: DefaultedString::::from_str("h").unwrap(), platform: Some("i".into()), + console: vec![ + Console::from_str("ttyS0").unwrap(), + Console::from_str("ttyS1,115200n8").unwrap(), + ], // skipped firstboot_args: None, append_karg: vec!["k".into(), "l".into()], diff --git a/src/cmdline/mod.rs b/src/cmdline/mod.rs index d8c7bc2dc..62c2095c2 100644 --- a/src/cmdline/mod.rs +++ b/src/cmdline/mod.rs @@ -18,12 +18,14 @@ use clap::{AppSettings, Parser}; use reqwest::Url; +mod console; #[cfg(feature = "docgen")] mod doc; mod install; mod serializer; mod types; +pub use self::console::*; #[cfg(feature = "docgen")] pub use self::doc::*; pub use self::install::InstallConfig; diff --git a/src/install.rs b/src/install.rs index faa2e321d..383a78282 100644 --- a/src/install.rs +++ b/src/install.rs @@ -394,6 +394,7 @@ fn write_disk( || !config.append_karg.is_empty() || !config.delete_karg.is_empty() || config.platform.is_some() + || !config.console.is_empty() || network_config.is_some() || cfg!(target_arch = "s390x") { @@ -404,7 +405,14 @@ fn write_disk( } if let Some(platform) = config.platform.as_ref() { write_platform(mount.mountpoint(), platform).context("writing platform ID")?; - write_console(mount.mountpoint(), platform).context("configuring console")?; + } + if config.platform.is_some() || !config.console.is_empty() { + write_console( + mount.mountpoint(), + config.platform.as_deref(), + &config.console, + ) + .context("configuring console")?; } if let Some(firstboot_args) = config.firstboot_args.as_ref() { write_firstboot_kargs(mount.mountpoint(), firstboot_args) @@ -558,42 +566,64 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> { } /// Configure console kernel arguments and GRUB commands. -fn write_console(mountpoint: &Path, platform: &str) -> Result<()> { - // anything to do? - if platform == "metal" { - return Ok(()); - } - +fn write_console(mountpoint: &Path, platform: Option<&str>, consoles: &[Console]) -> Result<()> { // read platforms table - let (spec, metal_spec) = match fs::read_to_string(mountpoint.join("coreos/platforms.json")) { - Ok(json) => { - let mut table: HashMap = - serde_json::from_str(&json).context("parsing platform table")?; - // no spec for this platform, or for metal? - ( - table.remove(platform).unwrap_or_default(), - table.remove("metal").unwrap_or_default(), - ) - } + let platforms = match fs::read_to_string(mountpoint.join("coreos/platforms.json")) { + Ok(json) => serde_json::from_str::>(&json) + .context("parsing platform table")?, // no table for this image? Err(e) if e.kind() == std::io::ErrorKind::NotFound => Default::default(), Err(e) => return Err(e).context("reading platform table"), }; + let mut kargs = Vec::new(); + let mut grub_commands = Vec::new(); + if !consoles.is_empty() { + // custom console settings completely override platform-specific + // defaults + let mut grub_terminals = Vec::new(); + for console in consoles { + kargs.push(console.karg()); + if let Some(cmd) = console.grub_command() { + grub_commands.push(cmd); + } + grub_terminals.push(console.grub_terminal()); + } + grub_terminals.sort_unstable(); + grub_terminals.dedup(); + for direction in ["input", "output"] { + grub_commands.push(format!("terminal_{direction} {}", grub_terminals.join(" "))); + } + } else if let Some(platform) = platform { + // platform-specific defaults + if platform == "metal" { + // we're just being asked to apply the defaults which are already + // applied + return Ok(()); + } + let spec = platforms.get(platform).cloned().unwrap_or_default(); + kargs.extend(spec.kernel_arguments); + grub_commands.extend(spec.grub_commands); + } else { + // nothing to do and the caller shouldn't have called us + unreachable!(); + } + // set kargs, removing any metal-specific ones + let metal_spec = platforms.get("metal").cloned().unwrap_or_default(); visit_bls_entry_options(mountpoint, |orig_options: &str| { KargsEditor::new() - .append(&spec.kernel_arguments) + .append(&kargs) .delete(&metal_spec.kernel_arguments) .maybe_apply_to(orig_options) .context("setting platform kernel arguments") })?; // set grub commands - if spec.grub_commands != metal_spec.grub_commands { + if grub_commands != metal_spec.grub_commands { let path = mountpoint.join("grub2/grub.cfg"); let grub_cfg = fs::read_to_string(&path).context("reading grub.cfg")?; - let new_grub_cfg = update_grub_cfg_console_settings(&grub_cfg, &spec.grub_commands) + let new_grub_cfg = update_grub_cfg_console_settings(&grub_cfg, &grub_commands) .context("updating grub.cfg")?; fs::write(&path, new_grub_cfg).context("writing grub.cfg")?; } From c32d8d2db0ab6de49a91f812fb2d00891ca52e26 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Wed, 7 Sep 2022 16:47:27 -0400 Subject: [PATCH 4/8] live: support missing feature flags If a flag was absent from features.json, we were failing to parse rather than defaulting the flag to false. This didn't matter in practice because both flags were introduced into production at the same time as the file in FCOS 35.20211215.2.0. --- src/live/customize.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/live/customize.rs b/src/live/customize.rs index c7cb063a6..8ba09dd7b 100644 --- a/src/live/customize.rs +++ b/src/live/customize.rs @@ -36,7 +36,7 @@ const COREOS_ISO_FEATURES_PATH: &str = "COREOS/FEATURES.JSO"; /// and /coreos/features.json in the live ISO. Written by /// cosa buildextend-live. #[derive(Default, Deserialize)] -#[serde(rename_all = "kebab-case")] +#[serde(default, rename_all = "kebab-case")] pub(super) struct OsFeatures { /// Installer reads config files from /etc/coreos/installer.d pub installer_config: bool, From cad56c5d6009f343d99cff89b67e7c5a619f05e9 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Thu, 8 Sep 2022 12:02:44 -0400 Subject: [PATCH 5/8] live: add --dest-console to pass through --console to installer Use the installer-config-directives features.json object introduced by https://github.com/coreos/coreos-assembler/pull/3083 to check whether the image's coreos-installer is new enough to support --console. --- docs/cmd/iso.md | 7 +++++++ docs/cmd/pxe.md | 7 +++++++ docs/customizing-install.md | 12 ++++++++++++ docs/release-notes.md | 1 + fixtures/customize/dest.bu | 2 ++ fixtures/customize/dest.ign | 2 +- man/coreos-installer-iso-customize.8 | 7 ++++++- man/coreos-installer-pxe-customize.8 | 7 ++++++- src/cmdline/mod.rs | 7 +++++++ src/live/customize.rs | 22 ++++++++++++++++++++++ tests/images/customize.sh | 22 ++++++++++++++++------ tests/images/unsupported.sh | 5 +++++ 12 files changed, 92 insertions(+), 9 deletions(-) diff --git a/docs/cmd/iso.md b/docs/cmd/iso.md index a29c58601..3dbe7aa57 100644 --- a/docs/cmd/iso.md +++ b/docs/cmd/iso.md @@ -34,6 +34,13 @@ OPTIONS: Automatically run installer, installing to the specified destination device. The resulting boot media will overwrite the destination device without confirmation. + --dest-console + Kernel and bootloader console for dest + + Automatically run installer, configuring the specified kernel and bootloader + console for the destination system. The argument uses the same syntax as the + parameter to the "console=" kernel argument. + --dest-karg-append Destination kernel argument to append diff --git a/docs/cmd/pxe.md b/docs/cmd/pxe.md index df1d59854..00d8c9c40 100644 --- a/docs/cmd/pxe.md +++ b/docs/cmd/pxe.md @@ -34,6 +34,13 @@ OPTIONS: Automatically run installer, installing to the specified destination device. The resulting boot media will overwrite the destination device without confirmation. + --dest-console + Kernel and bootloader console for dest + + Automatically run installer, configuring the specified kernel and bootloader + console for the destination system. The argument uses the same syntax as the + parameter to the "console=" kernel argument. + --dest-karg-append Destination kernel argument to append diff --git a/docs/customizing-install.md b/docs/customizing-install.md index d2cd4f99f..57bbe9958 100644 --- a/docs/customizing-install.md +++ b/docs/customizing-install.md @@ -62,6 +62,18 @@ Available customizations include: for Ignition to fetch remote resources. - Specifying HTTPS certificate authorities to be trusted by Ignition, in both the installed system and the live environment (`--ignition-ca`). +- Specifying consoles to be used by the installed system (`--dest-console`), + using the syntax of the `console` + [kernel argument](https://www.kernel.org/doc/html/latest/admin-guide/serial-console.html). + Consoles are configured for both the bootloader (GRUB) and the booted OS + (kernel). Consoles are subject to the Linux kernel rules: the first + specified console of each type is used, and the last specified console + is the primary console. + Supported graphical consoles are `tty0`, `hvc0`, and `ttysclp0`. + Supported serial consoles are `ttyS` and `ttyAMA`, with optional + baud rate, parity, and number of data bits. + Examples: `--console tty0`, `--console ttyAMA0,115200`, + `--console ttyS1,115200n8`. - Modifying kernel arguments of the installed system (`--dest-karg-append`, `--dest-karg-delete`) or the live ISO environment (`--live-karg-append`, `--live-karg-replace`, `--live-karg-delete`). These options are useful if diff --git a/docs/release-notes.md b/docs/release-notes.md index fdc3e0e92..7ff08a2f8 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -9,6 +9,7 @@ nav_order: 8 Major changes: - install: Add `--console` for configuring kernel and bootloader console +- customize: Add `--dest-console` for configuring kernel and bootloader console - Support reading initrd images compressed with zstd - Add Fedora 38 signing key; drop Fedora 35 signing key diff --git a/fixtures/customize/dest.bu b/fixtures/customize/dest.bu index 004b32f61..1e17035c9 100644 --- a/fixtures/customize/dest.bu +++ b/fixtures/customize/dest.bu @@ -18,10 +18,12 @@ systemd: ConditionKernelCommandLine=dest-karg ConditionKernelCommandLine=!ignition.platform.id=metal ConditionKernelCommandLine=ignition.platform.id=qemu + ConditionKernelCommandLine=console=ttyS0,115200n8 [Service] Type=oneshot RemainAfterExit=true + ExecStart=/bin/grep -qz 'serial --unit=0 --speed=115200 --word=8 --parity=no.terminal_input serial.terminal_output serial.#' /boot/grub2/grub.cfg ExecStart=/bin/echo @applied-dest-ign@ StandardOutput=tty diff --git a/fixtures/customize/dest.ign b/fixtures/customize/dest.ign index b8ece97a2..fa60b6a83 100644 --- a/fixtures/customize/dest.ign +++ b/fixtures/customize/dest.ign @@ -5,7 +5,7 @@ "systemd": { "units": [ { - "contents": "[Unit]\nDescription=Dest Ignition Applied\nBefore=multi-user.target\nConditionPathExists=/etc/NetworkManager/system-connections/installer-test.nmconnection\nConditionPathExists=/etc/NetworkManager/system-connections/nmstate-json-eth1.nmconnection\nConditionPathExists=/etc/NetworkManager/system-connections/nmstate-json-eth2.nmconnection\nConditionPathExists=/etc/NetworkManager/system-connections/nmstate-yaml-eth1.nmconnection\nConditionPathExists=/etc/NetworkManager/system-connections/nmstate-yaml-eth2.nmconnection\nConditionKernelCommandLine=install-config-karg-1\nConditionKernelCommandLine=install-config-karg-2\nConditionKernelCommandLine=dest-karg\nConditionKernelCommandLine=!ignition.platform.id=metal\nConditionKernelCommandLine=ignition.platform.id=qemu\n\n[Service]\nType=oneshot\nRemainAfterExit=true\nExecStart=/bin/echo @applied-dest-ign@\nStandardOutput=tty\n\n[Install]\nRequiredBy=multi-user.target\n", + "contents": "[Unit]\nDescription=Dest Ignition Applied\nBefore=multi-user.target\nConditionPathExists=/etc/NetworkManager/system-connections/installer-test.nmconnection\nConditionPathExists=/etc/NetworkManager/system-connections/nmstate-json-eth1.nmconnection\nConditionPathExists=/etc/NetworkManager/system-connections/nmstate-json-eth2.nmconnection\nConditionPathExists=/etc/NetworkManager/system-connections/nmstate-yaml-eth1.nmconnection\nConditionPathExists=/etc/NetworkManager/system-connections/nmstate-yaml-eth2.nmconnection\nConditionKernelCommandLine=install-config-karg-1\nConditionKernelCommandLine=install-config-karg-2\nConditionKernelCommandLine=dest-karg\nConditionKernelCommandLine=!ignition.platform.id=metal\nConditionKernelCommandLine=ignition.platform.id=qemu\nConditionKernelCommandLine=console=ttyS0,115200n8\n\n[Service]\nType=oneshot\nRemainAfterExit=true\nExecStart=/bin/grep -qz 'serial --unit=0 --speed=115200 --word=8 --parity=no.terminal_input serial.terminal_output serial.#' /boot/grub2/grub.cfg\nExecStart=/bin/echo @applied-dest-ign@\nStandardOutput=tty\n\n[Install]\nRequiredBy=multi-user.target\n", "enabled": true, "name": "dest-ignition-applied.service" } diff --git a/man/coreos-installer-iso-customize.8 b/man/coreos-installer-iso-customize.8 index e11bc8d7b..2a112118d 100644 --- a/man/coreos-installer-iso-customize.8 +++ b/man/coreos-installer-iso-customize.8 @@ -4,7 +4,7 @@ .SH NAME coreos\-installer\-iso\-customize \- Customize a CoreOS live ISO image .SH SYNOPSIS -\fBcoreos\-installer\-iso\-customize\fR [\fB\-h\fR|\fB\-\-help\fR] [\fB\-V\fR|\fB\-\-version\fR] [\fB\-\-dest\-ignition\fR] [\fB\-\-dest\-device\fR] [\fB\-\-dest\-karg\-append\fR] [\fB\-\-dest\-karg\-delete\fR] [\fB\-\-network\-keyfile\fR] [\fB\-\-network\-nmstate\fR] [\fB\-\-ignition\-ca\fR] [\fB\-\-pre\-install\fR] [\fB\-\-post\-install\fR] [\fB\-\-installer\-config\fR] [\fB\-\-live\-ignition\fR] [\fB\-\-live\-karg\-append\fR] [\fB\-\-live\-karg\-delete\fR] [\fB\-\-live\-karg\-replace\fR] [\fB\-f\fR|\fB\-\-force\fR] [\fB\-o\fR|\fB\-\-output\fR] <\fIISO\fR> +\fBcoreos\-installer\-iso\-customize\fR [\fB\-h\fR|\fB\-\-help\fR] [\fB\-V\fR|\fB\-\-version\fR] [\fB\-\-dest\-ignition\fR] [\fB\-\-dest\-device\fR] [\fB\-\-dest\-console\fR] [\fB\-\-dest\-karg\-append\fR] [\fB\-\-dest\-karg\-delete\fR] [\fB\-\-network\-keyfile\fR] [\fB\-\-network\-nmstate\fR] [\fB\-\-ignition\-ca\fR] [\fB\-\-pre\-install\fR] [\fB\-\-post\-install\fR] [\fB\-\-installer\-config\fR] [\fB\-\-live\-ignition\fR] [\fB\-\-live\-karg\-append\fR] [\fB\-\-live\-karg\-delete\fR] [\fB\-\-live\-karg\-replace\fR] [\fB\-f\fR|\fB\-\-force\fR] [\fB\-o\fR|\fB\-\-output\fR] <\fIISO\fR> .SH DESCRIPTION Customize a CoreOS live ISO image .SH OPTIONS @@ -25,6 +25,11 @@ Install destination device Automatically run installer, installing to the specified destination device. The resulting boot media will overwrite the destination device without confirmation. .TP +\fB\-\-dest\-console\fR=\fIspec\fR +Kernel and bootloader console for dest + +Automatically run installer, configuring the specified kernel and bootloader console for the destination system. The argument uses the same syntax as the parameter to the "console=" kernel argument. +.TP \fB\-\-dest\-karg\-append\fR=\fIarg\fR Destination kernel argument to append diff --git a/man/coreos-installer-pxe-customize.8 b/man/coreos-installer-pxe-customize.8 index b08c26528..d24a85614 100644 --- a/man/coreos-installer-pxe-customize.8 +++ b/man/coreos-installer-pxe-customize.8 @@ -4,7 +4,7 @@ .SH NAME coreos\-installer\-pxe\-customize \- Create a custom live PXE boot config .SH SYNOPSIS -\fBcoreos\-installer\-pxe\-customize\fR [\fB\-h\fR|\fB\-\-help\fR] [\fB\-V\fR|\fB\-\-version\fR] [\fB\-\-dest\-ignition\fR] [\fB\-\-dest\-device\fR] [\fB\-\-dest\-karg\-append\fR] [\fB\-\-dest\-karg\-delete\fR] [\fB\-\-network\-keyfile\fR] [\fB\-\-network\-nmstate\fR] [\fB\-\-ignition\-ca\fR] [\fB\-\-pre\-install\fR] [\fB\-\-post\-install\fR] [\fB\-\-installer\-config\fR] [\fB\-\-live\-ignition\fR] <\fB\-o\fR|\fB\-\-output\fR> <\fIpath\fR> +\fBcoreos\-installer\-pxe\-customize\fR [\fB\-h\fR|\fB\-\-help\fR] [\fB\-V\fR|\fB\-\-version\fR] [\fB\-\-dest\-ignition\fR] [\fB\-\-dest\-device\fR] [\fB\-\-dest\-console\fR] [\fB\-\-dest\-karg\-append\fR] [\fB\-\-dest\-karg\-delete\fR] [\fB\-\-network\-keyfile\fR] [\fB\-\-network\-nmstate\fR] [\fB\-\-ignition\-ca\fR] [\fB\-\-pre\-install\fR] [\fB\-\-post\-install\fR] [\fB\-\-installer\-config\fR] [\fB\-\-live\-ignition\fR] <\fB\-o\fR|\fB\-\-output\fR> <\fIpath\fR> .SH DESCRIPTION Create a custom live PXE boot config .SH OPTIONS @@ -25,6 +25,11 @@ Install destination device Automatically run installer, installing to the specified destination device. The resulting boot media will overwrite the destination device without confirmation. .TP +\fB\-\-dest\-console\fR=\fIspec\fR +Kernel and bootloader console for dest + +Automatically run installer, configuring the specified kernel and bootloader console for the destination system. The argument uses the same syntax as the parameter to the "console=" kernel argument. +.TP \fB\-\-dest\-karg\-append\fR=\fIarg\fR Destination kernel argument to append diff --git a/src/cmdline/mod.rs b/src/cmdline/mod.rs index 62c2095c2..5cb9c0327 100644 --- a/src/cmdline/mod.rs +++ b/src/cmdline/mod.rs @@ -265,6 +265,13 @@ pub struct CommonCustomizeConfig { /// device without confirmation. #[clap(long, value_name = "path")] pub dest_device: Option, + /// Kernel and bootloader console for dest + /// + /// Automatically run installer, configuring the specified kernel and + /// bootloader console for the destination system. The argument uses + /// the same syntax as the parameter to the "console=" kernel argument. + #[clap(long, value_name = "spec")] + pub dest_console: Vec, /// Destination kernel argument to append /// /// Automatically run installer, adding the specified kernel argument diff --git a/src/live/customize.rs b/src/live/customize.rs index 8ba09dd7b..34e0b5747 100644 --- a/src/live/customize.rs +++ b/src/live/customize.rs @@ -40,10 +40,18 @@ const COREOS_ISO_FEATURES_PATH: &str = "COREOS/FEATURES.JSO"; pub(super) struct OsFeatures { /// Installer reads config files from /etc/coreos/installer.d pub installer_config: bool, + /// Directives supported in installer config files + pub installer_config_directives: InstallerDirectives, /// Live initrd reads NM keyfiles from /etc/coreos-firstboot-network pub live_initrd_network: bool, } +#[derive(Default, Deserialize)] +#[serde(default, rename_all = "kebab-case")] +pub(super) struct InstallerDirectives { + pub console: bool, +} + impl OsFeatures { pub fn for_iso(iso: &mut IsoFs) -> Result { match iso.get_path(COREOS_ISO_FEATURES_PATH) { @@ -97,6 +105,9 @@ impl LiveInitrd { if let Some(path) = &common.dest_device { conf.dest_device(path)?; } + for arg in &common.dest_console { + conf.dest_console(arg)?; + } for arg in &common.dest_karg_append { conf.dest_karg_append(arg); } @@ -146,6 +157,17 @@ impl LiveInitrd { Ok(()) } + pub fn dest_console(&mut self, console: &Console) -> Result<()> { + if !self.features.installer_config_directives.console { + bail!("This OS image does not support customizing the destination console."); + } + self.installer + .get_or_insert_with(Default::default) + .console + .push(console.clone()); + Ok(()) + } + pub fn dest_karg_append(&mut self, arg: &str) { self.installer .get_or_insert_with(Default::default) diff --git a/tests/images/customize.sh b/tests/images/customize.sh index 4958e7e79..e321f9133 100755 --- a/tests/images/customize.sh +++ b/tests/images/customize.sh @@ -31,14 +31,19 @@ grepq() { # kargs we need on every boot kargs_common=( - # make sure we get log output - console=ttyS0,115200 - # make sure it's readable + # make sure log output is readable systemd.log_color=0 # force NM in initrd rd.neednet=1 ) +# kargs we need on live boots +kargs_live=( + # make sure we get log output + # the installed system sets this with --dest-console instead + console=ttyS0,115200 +) + # options that don't initiate an install, even if they pertain to one opts_common=( # @applied-live-ign@ @@ -75,6 +80,8 @@ opts_install=( # Tested implicitly --dest-device "/dev/vda" # Condition of @applied-dest-ign@ + --dest-console "ttyS0,115200" + # Condition of @applied-dest-ign@ --dest-karg-append dest-karg # Condition of @applied-dest-ign@ --dest-karg-delete ignition.platform.id=metal @@ -86,7 +93,7 @@ for arg in ${kargs_common[@]}; do done opts_iso=() -for arg in ${kargs_common[@]}; do +for arg in ${kargs_common[@]} ${kargs_live[@]}; do opts_iso+=(--live-karg-append "${arg}") done @@ -123,7 +130,7 @@ qemu_iso() { qemu_pxe() { cat > ipxe < Date: Fri, 9 Sep 2022 01:39:37 -0400 Subject: [PATCH 6/8] tests: automatically run individual tests on all fixtures they support Rather than manually adding new fixtures to every test individually, have each test declare its minimum supported fixture, and then automatically run the test on any newer fixtures. Fixes failure to invoke some tests on 2022-02 fixture. --- tests/images.sh | 42 ++++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/tests/images.sh b/tests/images.sh index f70bbfa27..b9170d299 100755 --- a/tests/images.sh +++ b/tests/images.sh @@ -3,7 +3,15 @@ set -euo pipefail export PATH="$(realpath $(dirname $0)/../target/${PROFILE:-debug}):$PATH" -fixtures="$(realpath $(dirname $0)/../fixtures)" +fixturesdir="$(realpath $(dirname $0)/../fixtures)" + +fixtures=( + embed-areas-2020-09.iso.xz + embed-areas-2021-01.iso.xz + embed-areas-2021-09.iso.xz + embed-areas-2021-12.iso.xz + embed-areas-2022-02.iso.xz +) msg() { cat < Date: Fri, 9 Sep 2022 01:45:14 -0400 Subject: [PATCH 7/8] fixtures/iso: add fixture with installer-config-directives from 0.16.0 In particular, it includes the "console" directive. --- fixtures/iso/INDEX | 7 +++++++ fixtures/iso/embed-areas-2022-09.iso.xz | Bin 0 -> 7064 bytes tests/images.sh | 1 + 3 files changed, 8 insertions(+) create mode 100644 fixtures/iso/embed-areas-2022-09.iso.xz diff --git a/fixtures/iso/INDEX b/fixtures/iso/INDEX index 468ccfaac..688d2bcf8 100644 --- a/fixtures/iso/INDEX +++ b/fixtures/iso/INDEX @@ -25,3 +25,10 @@ embed-areas-2022-02.iso.xz kargs.json pointing to kargs areas and embedding defaults miniso.dat version 1 features.json: installer-config, live-initrd-network + +embed-areas-2022-09.iso.xz + kargs.json pointing to kargs areas and embedding defaults + miniso.dat version 1 + features.json: installer-config, installer-config-directives, + live-initrd-network + installer-config-directives from 0.16.0 diff --git a/fixtures/iso/embed-areas-2022-09.iso.xz b/fixtures/iso/embed-areas-2022-09.iso.xz new file mode 100644 index 0000000000000000000000000000000000000000..c82b9758306df19f5963f9a3f7e693f65e91ee54 GIT binary patch literal 7064 zcmV;J8)xMGH+ooF000E$*0e?f03iV!0000G&sfaw|Nk3UT>u%oP8)^5PkkcPL#046 zvn9ybp7Gsc!1wYBpCLDa^+$C39`SNnZ!O`WEbAL9?UFPf7|5-5c-oABV`3|QWaioE zZkpLs*3vut<-v)-=Sa;bb#jK=D4Vft{8)!AAJ0^L>cCh3mFm%>2lzm$-dUvbm%(FT zlij5Ne$z^=ovrB_ibhs;(e2r~2(`|}a)O?bvf~FJ94_4-yZOU(8-Mz?%;QQn<60-@ zO0!2lQCT+R1LFxzFf(v?5TVMnI#JwCFxN`=R|h|cO}p=*@L7PC8Vx!UUAlG6k_B`i z@y>dgOLKUKr_+Y{?18l0y0KW10*iAM zATErWXb_glO{7;8N7K4N5NS4!R~Q`{a`hY;JEP@%|DI*iobB6-F^j{bjm_cze3OWx z=Nh{tZl1=)IWci36I4Pst+h|6Lph-!+tW0zd6^U*x6t@*UWP-$!B6SeSJ8Vy^WXpE z@W6ow2VyYVZ`nF-YWK|1Gz-$owNEo9tPY<+ZO9P56-dzfG^ca#|IoKC9UXk4|8Hzt z;e!__#<2Ot_?o6C)p+kRpf`oDdYu6xUbbz=zH}b*!!-NgLmM<*6dANFRya*I>`}K` zQu++}QCT9@xE|b7zuK;h6HNiJ#PVVPlP;zp!bS$C5$W)#W5W*vE)>e8Yj}BJ;qQ-j z^lE?+yJLcEPTEOB>?j)OUD6qf6K0Y02-Pz(TeEq)%=)-W`~>3{<;>IS8Glch13%mV z&}=@EQzcLYuo!$MoolX0hm>v)7Te-=36{sP=s@(rHM>3_{WzJ80R(4F4`1BHn0AdT zj3|wbG{otTV&0uSvdn3q94#v~D=G0%8#qiT72>3J^&7Mm=tz~EI{xEA2}SZc;V{uD z%yoN|kfMBt0z;uLzk#{G(QDF3hAG!ERZ;hf8H8X1+{pB`SSOSlv&k!X1W;5b=yG?7 zur2Arsl{*DVaCsiP0UJ0?O3N_7-b9n8|QI^+V@L1+%r4EJG@5oHGakajz&uSex~NR z0l!Zo(fbhOo0w~|^t(1HG2?qRApa6E?~tEzpY|CFvj>L2U6-=Q10W9x+*FI^#Ntw@ zWx0rF^SjrE=je>`4a%P%9rXNEz~ePUZ1VT}K0eeu8ZtaO3@BPcwftm_7Ahg+`zq&3 zaydYhCWx>!&HeY`-WMxPb5nz6{;CoFuEE{yQ$t2ZBX7rpWo@wsu=SN^=+A`nMK_P} zCT~|#Z~@EaxHKxo8j<)ib;%rR$dt0F%BcVG0`l6B4rA_K zmnU&s1kFWNKk}O&D2hCf6f%@$Xb`W?N6EM ztifvrXqZd|VRsP0nlIaEoKIHtkmT{2fi|9mi3H%{D~6*q&t(vA5s;``mh?>U>w{pO z$g)QoSvdy!Bd40W6V{EJp|f`SJY}BOxaXhH0q)vJ%osqvSZZYhbN-qL|Np2tLmI)( zy$rV;x28=`2@9In&_onCP_BOL3xkw<4-n!{SFqw25f!6p&lvYT>;8(`XriWi z9C6Vs14uMq04a^{HIeDua^Oc?QrRu>Y2u*@6YimmL#w$Z$3t~hYy?0jc}HYmsX81* z`8pJ}zzS*m$SP_d5f-h27bU+uoYX=)wmj;_zMq63H@!=EgfxS24bDu^_`tsYjpLsi8&vH;wMx64=yMAt=R~5YmtNGjFYx~5C zX12e%(z&#fXDM*J^Qkc^=pWlwon9P9@Ugr0=M1ZZ0mb zSr`ls!Y&VZupHO4MGtsXpigK%f{#ZW1|p8bQPhIyHJ@nsmXlvkOKrhG9!(s+w)LJ- zeb9oS8K&Y&H0LyuL6UvX8QGlOSzagtU4@NuOGoE!5>AFTFM0V(P_OKenLH7 zwCtVZLF-TeP}fdAKSkNk2+*m&=)$YAH#>g;qb8N_W46f+NfAKRhpk?E#mr&3z8~)& zp2W1aqddizLj3Y;3lZuyXWvDwFtP(nw3O%iF4IlPB4<25~Ba|K~ zLQCd-^O0@fMg}(kBFS;p$_|PK`>++rrL;WMYH;DV(|*NlvNn?O<0#(lGu&~JrZQ{S z?;D}ww@g)UpWlN$R;{_y3nVc*S1$-#7r=yRnZU|_cl<;eHUt95U#YY^EAGLO4F*sh zbT*G_qv|7;|16G9P8+M_D2X4d8Cf`RAq!gCIZqa%oxfQ+p&7r3k{s9AzHKUtvuizJ zV^dCSt=y9AiYBahwWfVR1L>7h20xu)dAvJPoIDo!jGRWgtEFO`Vz5G0lhpc~#;m;S zDljXkRCtK;&c}X7&poX&)Q4k1Tu7{KhJ^+rM^>%1bU#p{gk5%Xhekg zW4^kK7WNu?f_!Lz9H|XtVrPot2r1o<@)1$ua-xvOPB~yb(ZKmV7Qcib{Y=!X(eyz@ zu#~W>A6fvY>t;2Uf)Ge`dieCO1`N^n7fN;Y-xu;Lv73o)ma~Nv$2;yMfl$0gL*POi z8;i%%`rZlIleT!(;t9tM4~&EKiUis#+*+Z!vb!dbD8OGED(iEvqk9?D^@(#~TNMvZ z5z zILASb4Z|b{h{|aTf$ZI7^hY#}>`78@ni~^BX&TrN1#?xFTet+S0X)?pEozU! zr&+B6-Y7Y@Y1rKIY|8Jq5uh%s)u7lIR$!U%B?VTFQA9vB^)%plXAdAH5;G^hH-F!d zzdAttyEH|A=5=+cFadX@cr7)bOP^M_@m4o`J$*ZUeF)T8&RbIM20to-cIp1)#W$Xw z^|@=&aB?t|J)^N<2^8by%Rwe$iiFHGcW73SnSr$Ai^G~tgwgG%NmL#R>-SQsXs|1# z>LceeF+UULco9JR^2@JJORv=sqjy*DtAYWu_P@C~bQ@NTPYW16xxAV7Em?Ke59%>Q z!2PK6k+Y&$AKyV=ln|!gRWfr~4y_felC414As#nPz`U@S3thypaKiUvyG2#e0qNTz z-flr}3Wna7s44)p!%$PO3_?VEgr#-ca%_#l9Y25@^3kZ%edv@p9?GR?=FH;PG!FIXa+5ICgOJQ*Jo7j`hNg3RKHvntdx2;RoN$! z29e?kG}6<;dOboV)OcLb27ff%Z{rG5rtO~RZ$efI3Ju+)1!O3&Pspd+c|mLHDy&1u9e2FXbqT`rE-{1lrdg0vc^kZhRT0^9Od(?c zxVk#`QO5!QSX7-Ba7lh2>(}JCT5{mnNSo!q&%$LHc8Vpj7oD#!x|gMXS?1)xMqR#w zLS^rs$|Gd(Rvxsa9_5*^3OB??Jx$zY-hDnDybdM@7ivXUkNHji9rsQo4LazXREv;S zll(V_Cs>9)!q5Aj^XzXtcQB7GbYxzZ-rU7fn@h7gagfaWr3Qz8on56rLbi>~qEWP1 zjzPzSx92cb1I=v2GP0FrY|b+-8?ptX!RREdt(XvU;6d2hd%0?17<-G}zkoh@fezp4 zo0%OuoYJlpL`P;7_7w9TKKAuvso|l+-d!MkwC`B54nq7T4$RsKkANxN8f_jB&z)4rrAgc_w$*69DFjS1Oy#^a3!Clh|qx@{#pPwb5%i69F1X zYm;;Y=iKC1)3O)URO2e0nvg{RcqqdUesJo*JG9ggH+wbKrdH={v{>NaQ`;Q>LAF1= znbU_%TF1U1om45mc3W<|CE-*PcQLHckH1iybSlKDBvK&cRw#?Ime$?y8?xO?P>?@n zvow{nqlsD^Wt-1KfC|BmYj6_;Hq8I@n{Fudv7FTJ7Rx(*< z;3Sf1Qz-g;fX-+BplvF6a35wo^(n)rA7|7KKQ7y}C*9{QY1`W9zQ1Mr*cDIxMVwy* zfl>oL!Np7{1h7G^k)L+`6X92xt61h^6+Wib-)CQ5mli__LHanbO3HG{fC!)vp(G=u zWQ-=HWy?R*ID!bx`5xE6Xm~#7yX%KlOyC*SpI=+Ctct^=9?}&D0w03Z!_18JIthXF zzB3CR?lfD~Smv`MFeyrxhTVsA3-refv@;G=cijZ7VpZ`_?G0;do&NqcvGMw9YweMy za+gGb;z@P$*TEHGIG*aSuWkmp?wi;}UXsbG#>YrZ{>>n9SQz*!p zKrC(PS4v?hXu6o+$+^UK0<*`j-1bI${E+Ht%`z8P)+a!Q!_kr5PNyKtu zl$R+TDS*&;#7Z}}R1wf0J_=re2y^y0@47nL8jJP(recw0w9H6gmZXl{fKbUqujz7I z-UD7}mhAVj&yeAP*URs2hrVxUaXg`kwL|hFs|f9_k;*LYwsP$?7jK6)v5nCCm=)Uc zg|C9#7vcDFHj&XE8 zvRK>M$2RE-m}rCWeE8aR@4K^I;GLAMColIABAiku+aSyo=Y~X!`L-txEjif2sqm_& zuMdiK?k2F(VbsNCS`99dP?O976dW5Jr=;HOqv@xP-YNIu40#)^Kwwk|#KVvxK_b%E zJs1N=W>4o9-dX@O2qoNb*gv?T_%r?IFr?ODqW~3N^;wcafa8+dq>pq^I*oIXmWM~I zH0KMH)HGsJ#Jn_Z8AlFVu@sGQmH2EI4R%vCgnRDdf~8ye6cbWY5q9gk9(!WM<&~!F z_%jA9EdSksB`Nqz2ebN=R<)?ch)21TtaS-9gt()q8p1dAl zUHVC}@BW~c?xplDF=Aaa4022;pLZdbY*XS--#M*P zPBY?2Ualt~qZAB}M9w^BD*(|nQNlga)OHlYMc(1BH(^tluTXIdJM!9KvW;(T{!d

)7W%76LGfuZ7gs`Oao4%nbI}BJqIkKbIc~IL0}5FLoXD|3z16mOQ2_AC>^G?znD?m~ki6JB9jpGpwK!y<*;fQOQGO)38Nn9J%Ec(4J#EZ4?{wZ zW>y+_=F1OEIiUxK8hLyK#C)6rxA|=}68~{t*>9C4V5-Uh85WE_zZ<8vct}D!V3_y< z8o_oH?-W`auDNfBqZZV2X*KBz&0|HYjOI}eZR%qLW5>SD0?H%87GNe7ye&CiOMx8S zwWnq1*ux{TOr`u_ZnPU*Z4C1wo0Bl~lZOsb&>W`~Ar{@8_%2oVynw8UG@aS(sVNQ( zy?OuASKR|Rd4^0cG|WlHv{P^_QkflkfLB1J#^0*mBGS(%2uQ=flgdq!^5BcOH12tM zGCNOM#Lk0GikFz7x@xE@io6b&)+GvzzwH<^uFM~$>R-PrFW2oT%Q8{Xr}V*DmnM9` zX0KEcdy423XKuGo{XFbkKo&dr5BzEngDUHORK9_&BP)6vv0b$1@KHCR!C*z5mC*cA&QX0U?sS427}xL17x(NwyKxekcnvqV!X(OOtGyoGx7d)d z8>mNZvylFhUPCTUM1&kFlqN||eP)D@3(Gu!;(wB-@3oH!m^=TDVJhei?%V6Ap=e^a zfBwvfhjTD!q+6oxMZv+E0ElIoi8)+HMeeSfbAT>VbkrkP+4AO$4B_>qFxD@-(!-Hf zO9|SgiUjVT-lamzh(BBakI+u9CMc9~A0V_pX%nQi5^M`(MfKDZ#{(CaSF$%#z4Fpr z(~q<%*yVK5lv7EegB?x3&WJfe6Yi@cL3a(!XOswuGO27Z%8)v+RM>z_VR3mhJaUhI z=}1pqjhvc?t^bef76eTo!aD)gyaCY}K4d(le3r>W?r)HJx!6qB#TcXcLXx&T`8n)U zisZf!DP|Yo6$y_Xo|rs#^=IJ(cNXCi1i-3NE}LO=0gBkd#I_~4$d3x@{a<$o^9SPZ zr@rR8obmn8uYj^lQF#<#C37(rXtnXuDRFd~cwB?~t1{Hf2bYUfh52|E^Nf9RkiJu~ zPDRt2R#U?kE}3bj83u}+jR%$X{QqkjBgw~^uI54(HqYn48j4#FTQ9*tJmRpw^wCZu zOBs$k)k*nQHcD~Y%-46oGT~zrg-u&bicP0_coIlLSjut)bv>~eq+ui_SWRowUDgU9 z{P|0$+pu;RQCL^6@Z@CNSE#l}A=6M5exetNbK*6MiWpN0BaoC4?J--ZTGMz;1I+` zzUg+XS>)wg1qk`a+O7QiQY&=L9$O$n`Q1}J_$g!wJAkj5MjADW;Sm0`-zxGz4iOBA z1pyutG>e`nCUo1L=UQq7(f|O!gOuSI|9yS{0rWP2fIt9BAR?c!#Ao{g000001X)`B C__ad- literal 0 HcmV?d00001 diff --git a/tests/images.sh b/tests/images.sh index b9170d299..1974ff529 100755 --- a/tests/images.sh +++ b/tests/images.sh @@ -11,6 +11,7 @@ fixtures=( embed-areas-2021-09.iso.xz embed-areas-2021-12.iso.xz embed-areas-2022-02.iso.xz + embed-areas-2022-09.iso.xz ) msg() { From c8a24ea80d75b5447de386a85e279dd3c9ab70ad Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Sat, 10 Sep 2022 00:46:51 -0400 Subject: [PATCH 8/8] Warn on console= karg if --console could have been used instead If one or more console= kargs are specified to the install or iso/pxe customize subcommands, and Console can successfully parse all of those kargs, warn the user that they might benefit from specifying them using the console mechanism instead. For legibility, wrap the message at 80 characters, but avoid wrapping in the middle of the example argument lists. Use the textwrap crate to do this. textwrap is already a clap dependency without default features, and we also avoid non-default features here, so this doesn't increase the binary weight. --- Cargo.lock | 1 + Cargo.toml | 1 + docs/release-notes.md | 4 +- src/cmdline/console.rs | 87 ++++++++++++++++++++++++++++++++++++++++++ src/install.rs | 1 + src/live/customize.rs | 5 +++ 6 files changed, 98 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 4b86e3a68..f8aec9f48 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -235,6 +235,7 @@ dependencies = [ "serde_with", "serde_yaml", "tempfile", + "textwrap", "thiserror", "url", "uuid 1.1.2", diff --git a/Cargo.toml b/Cargo.toml index 07310c6c6..f288ce6eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -77,6 +77,7 @@ serde_json = "^1.0" serde_with = ">= 1.9.4, < 2" serde_yaml = ">= 0.8, < 0.10" tempfile = ">= 3.1, < 4" +textwrap = { version = ">= 0.15.0, < 0.16.0", default-features = false } thiserror = "1.0" url = ">= 2.1, < 3.0" uuid = { version = ">= 0.8, < 2.0", features = ["v4"] } diff --git a/docs/release-notes.md b/docs/release-notes.md index 7ff08a2f8..d5e9a4579 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -20,6 +20,7 @@ Minor changes: - Fix unlikely decompression error reading initrd - Add release notes to documentation - iso: Detect incomplete ISO files +- Warn if console kargs could have used `--console`/`--dest-console` instead Internal changes: @@ -30,7 +31,8 @@ Internal changes: Packaging changes: - Require Rust ≥ 1.58.0 -- Add dependency on `zstd` crate and `libzstd` shared library +- Add dependencies on `textwrap` and `zstd` crates +- Add dependency on `libzstd` shared library - Support `serde_yaml` 0.9 - Remove non-Linux dependencies from vendor archive - Install example installer config file in `/usr/share/coreos-installer` diff --git a/src/cmdline/console.rs b/src/cmdline/console.rs index 72eae361e..4f542d620 100644 --- a/src/cmdline/console.rs +++ b/src/cmdline/console.rs @@ -94,6 +94,55 @@ impl Console { pub fn karg(&self) -> String { format!("{KARG_PREFIX}{}", self) } + + /// Write a warning message to stdout if kargs contains "console=" + /// arguments we can parse and no "console=" arguments we can't. The + /// warning suggests that users use console_option instead of + /// karg_option to specify the desired console. + pub fn maybe_warn_on_kargs(kargs: &[String], karg_option: &str, console_option: &str) { + use textwrap::{fill, Options, WordSplitter}; + if let Some(args) = Self::maybe_console_args_from_kargs(kargs) { + // automatically wrap the message, but use Unicode non-breaking + // spaces to avoid wrapping in the middle of the argument + // strings, and then replace the non-breaking spaces afterward + const NBSP: &str = "\u{a0}"; + let msg = format!( + "Note: consider using \"{}\" instead of \"{}\" to configure both kernel and bootloader consoles.", + args.iter() + .map(|a| format!("{console_option}{NBSP}{a}")) + .collect::>() + .join(NBSP), + args.iter() + .map(|a| format!("{karg_option}{NBSP}console={a}")) + .collect::>() + .join(NBSP), + ); + let wrapped = fill( + &msg, + Options::new(80) + .break_words(false) + .word_splitter(WordSplitter::NoHyphenation), + ) + .replace(NBSP, " "); + eprintln!("\n{}\n", wrapped); + } + } + + /// If kargs contains at least one console argument and all console + /// arguments are parseable as consoles, return a vector of verbatim + /// (unparsed) console arguments with the console= prefixes removed. + fn maybe_console_args_from_kargs(kargs: &[String]) -> Option> { + let (parseable, unparseable): (Vec<&str>, Vec<&str>) = kargs + .iter() + .filter(|a| a.starts_with(KARG_PREFIX)) + .map(|a| &a[KARG_PREFIX.len()..]) + .partition(|a| Console::from_str(a).is_ok()); + if !parseable.is_empty() && unparseable.is_empty() { + Some(parseable) + } else { + None + } + } } impl FromStr for Console { @@ -257,4 +306,42 @@ mod tests { Console::from_str(input).unwrap_err(); } } + + #[test] + fn maybe_console_args_from_kargs() { + assert_eq!( + Console::maybe_console_args_from_kargs(&[ + "foo".into(), + "console=ttyS0".into(), + "bar".into() + ]), + Some(vec!["ttyS0"]) + ); + assert_eq!( + Console::maybe_console_args_from_kargs(&[ + "foo".into(), + "console=ttyS0".into(), + "console=tty0".into(), + "console=tty0".into(), + "console=ttyAMA1,115200n8".into(), + "bar".into() + ]), + Some(vec!["ttyS0", "tty0", "tty0", "ttyAMA1,115200n8"]) + ); + assert_eq!( + Console::maybe_console_args_from_kargs(&[ + "foo".into(), + "console=ttyS0".into(), + "console=ttyS1z".into(), + "console=tty0".into(), + "bar".into() + ]), + None + ); + assert_eq!( + Console::maybe_console_args_from_kargs(&["foo".into(), "bar".into()]), + None + ); + assert_eq!(Console::maybe_console_args_from_kargs(&[]), None); + } } diff --git a/src/install.rs b/src/install.rs index 383a78282..cd7abc904 100644 --- a/src/install.rs +++ b/src/install.rs @@ -421,6 +421,7 @@ fn write_disk( if !config.append_karg.is_empty() || !config.delete_karg.is_empty() { eprintln!("Modifying kernel arguments"); + Console::maybe_warn_on_kargs(&config.append_karg, "--append-karg", "--console"); visit_bls_entry_options(mount.mountpoint(), |orig_options: &str| { KargsEditor::new() .append(config.append_karg.as_slice()) diff --git a/src/live/customize.rs b/src/live/customize.rs index 34e0b5747..416792e42 100644 --- a/src/live/customize.rs +++ b/src/live/customize.rs @@ -108,6 +108,11 @@ impl LiveInitrd { for arg in &common.dest_console { conf.dest_console(arg)?; } + Console::maybe_warn_on_kargs( + &common.dest_karg_append, + "--dest-karg-append", + "--dest-console", + ); for arg in &common.dest_karg_append { conf.dest_karg_append(arg); }