Skip to content

Commit

Permalink
put diagnostics behind a default feature
Browse files Browse the repository at this point in the history
  • Loading branch information
gjcolombo committed Oct 17, 2024
1 parent e498df8 commit 89bc90a
Show file tree
Hide file tree
Showing 6 changed files with 18 additions and 26 deletions.
10 changes: 7 additions & 3 deletions .github/buildomat/jobs/image.sh
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,16 @@ rustc --version

banner build

# Enable the "omicron-build" feature to indicate this is an artifact destined
# for production use on an appropriately configured Oxide machine
# This job produces the propolis-server zone image that the Omicron repository
# imports to create full-blown Oxide software packages. Disable all default
# features and opt into the "omicron-build" feature instead to build libraries
# in their "production" configurations.
#
# The 'release' profile is configured for abort-on-panic, so we get an
# immediate coredump rather than unwinding in the case of an error.
ptime -m cargo build --release --verbose -p propolis-server --features omicron-build
ptime -m cargo build --release --verbose -p propolis-server \
--no-default-features \
--features omicron-build

banner image
ptime -m cargo run -p propolis-package
Expand Down
2 changes: 1 addition & 1 deletion bin/propolis-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ expectorate.workspace = true
mockall.workspace = true

[features]
default = []
default = ["propolis/dump-guest-state"]

# When building to be packaged for inclusion in the production ramdisk
# (nominally an Omicron package), certain code is compiled in or out.
Expand Down
1 change: 0 additions & 1 deletion bin/propolis-server/src/lib/vcpu_tasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ impl VcpuTasks {
VmEntry::Run
}
VmExitKind::InstEmul(inst) => {
// TODO(gjc) the context contains guest data!
let diag = propolis::vcpu::Diagnostics::capture(vcpu);
error!(log,
"instruction emulation exit on vCPU {}",
Expand Down
1 change: 1 addition & 0 deletions lib/propolis/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ rand.workspace = true
[features]
default = []
crucible-full = ["crucible", "crucible-client-types", "oximeter", "nexus-client"]
dump-guest-state = []
falcon = ["libloading", "p9ds", "dlpi", "ispf", "rand", "softnpu", "viona_api/falcon"]

# TODO until crucible#1280 is addressed, enabling Nexus notifications is done
Expand Down
6 changes: 3 additions & 3 deletions lib/propolis/src/exits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,10 @@ pub struct InstEmul {

impl std::fmt::Debug for InstEmul {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
#[cfg(feature = "omicron-build")]
let inst_data = "<redacted>";
#[cfg(not(feature = "dump-guest-state"))]
let inst_data = "<dump-guest-state disabled>";

#[cfg(not(feature = "omicron-build"))]
#[cfg(feature = "dump-guest-state")]
let inst_data = &self.inst_data;

f.debug_struct("InstEmul")
Expand Down
24 changes: 6 additions & 18 deletions lib/propolis/src/vcpu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1344,33 +1344,28 @@ mod bits {

/// Pretty-printable diagnostic information about the state of a vCPU.
pub struct Diagnostics {
/// True if capturing vCPU diagnostics is disabled by the library's
/// build-time configuration. This is used to prevent vCPU data from
/// appearing in logs when building production Propolis binaries.
disabled_by_config: bool,
gp_regs: Result<migrate::VcpuGpRegsV1>,
seg_regs: Result<migrate::VcpuSegRegsV1>,
ctrl_regs: Result<migrate::VcpuCtrlRegsV1>,
}

impl Diagnostics {
#[cfg(not(feature = "omicron-build"))]
#[cfg(feature = "dump-guest-state")]
pub fn capture(vcpu: &Vcpu) -> Self {
Self {
disabled_by_config: false,
gp_regs: migrate::VcpuGpRegsV1::read(vcpu),
seg_regs: migrate::VcpuSegRegsV1::read(vcpu),
ctrl_regs: migrate::VcpuCtrlRegsV1::read(vcpu),
}
}

#[cfg(feature = "omicron-build")]
#[cfg(not(feature = "dump-guest-state"))]
pub fn capture(_vcpu: &Vcpu) -> Self {
let msg = "dump-guest-state feature disabled";
Self {
disabled_by_config: true,
gp_regs: Err(std::io::Error::from(std::io::ErrorKind::Other)),
seg_regs: Err(std::io::Error::from(std::io::ErrorKind::Other)),
ctrl_regs: Err(std::io::Error::from(std::io::ErrorKind::Other)),
gp_regs: Err(std::io::Error::new(std::io::ErrorKind::Other, msg)),
seg_regs: Err(std::io::Error::new(std::io::ErrorKind::Other, msg)),
ctrl_regs: Err(std::io::Error::new(std::io::ErrorKind::Other, msg)),
}
}
}
Expand Down Expand Up @@ -1437,13 +1432,6 @@ impl std::fmt::Display for migrate::VcpuCtrlRegsV1 {

impl std::fmt::Display for Diagnostics {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
if self.disabled_by_config {
return writeln!(
f,
"vCPU diagnostics disabled by build configuration"
);
}

writeln!(f)?;
match &self.gp_regs {
Ok(regs) => {
Expand Down

0 comments on commit 89bc90a

Please sign in to comment.