Skip to content

Commit

Permalink
Ignore EPIPE in CLI (#746)
Browse files Browse the repository at this point in the history 
Piping command output to a process that performs a partial read before
closing the pipe, such as `head`, will cause an `EPIPE` to be raised on
the next write attempt. The standard `(e)print(ln)!` macros will panic
on any errors when writing, including `EPIPE`. One option to handle this
is to switch to `write(ln)!`, but this will inject new requirements to
handle `Result` where used, which can be onerous.

Create wrappers around the print macros to ignore `EPIPE`, and replace
calls to the originals with them. Update `main` to ignore any `EPIPE`s
returned from `writeln!` calls in subcommands.

We deliberately do not make this change to the `auth login/logout`
subcommands as these are mutating the config. Failure to notify the user
of the changes is fatal.

Before:

  $ oxide system networking switch-port-settings show | head -1
    switch0/qsfp0
    thread 'tokio-runtime-worker' panicked at library/std/src/io/stdio.rs:1021:9:
    failed printing to stdout: Broken pipe (os error 32)
    note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
    thread 'main' panicked at cli/src/main.rs:102:10:
    called `Result::unwrap()` on an `Err` value: JoinError::Panic(Id(15), ...)

After:

  $ oxide system networking switch-port-settings show | head -1
    switch0/qsfp0
  • Loading branch information
@wfchandler
wfchandler authored Jul 17, 2024
1 parent 040f1b0 commit f472bce
Show file tree
Hide file tree
Showing 6 changed files with 183 additions and 54 deletions.
26 changes: 14 additions & 12 deletions cli/src/cmd_api.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use futures::{StreamExt, TryStreamExt};
use oxide::Client;
use serde::Deserialize;

use crate::{print_nopipe, println_nopipe};

/// Makes an authenticated HTTP request to the Oxide API and prints the response.
///
/// The endpoint argument should be a path of a Oxide API endpoint.
Expand Down Expand Up @@ -183,18 +185,18 @@ impl crate::AuthenticatedCmd for CmdApi {
if !resp.status().is_success() {
let status = resp.status();
let v = resp.json::<serde_json::Value>().await?;
println!(
println_nopipe!(
"error; status code: {} {}",
status.as_u16(),
status.canonical_reason().unwrap_or_default()
);
println!("{}", serde_json::to_string_pretty(&v).unwrap());
println_nopipe!("{}", serde_json::to_string_pretty(&v).unwrap());
return Err(anyhow!("error"));
}

// Print the response headers if requested.
if self.include {
println!("{:?} {}", resp.version(), resp.status());
println_nopipe!("{:?} {}", resp.version(), resp.status());
print_headers(resp.headers())?;
}

Expand All @@ -205,7 +207,7 @@ impl crate::AuthenticatedCmd for CmdApi {
if !self.paginate {
// Print the response body.
let result = resp.json::<serde_json::Value>().await?;
println!("{}", serde_json::to_string_pretty(&result)?);
println_nopipe!("{}", serde_json::to_string_pretty(&result)?);

Ok(())
} else {
Expand Down Expand Up @@ -239,7 +241,7 @@ impl crate::AuthenticatedCmd for CmdApi {
Result::Ok(Some((items, next_page)))
});

print!("[");
print_nopipe!("[");

let result = first
.chain(rest)
Expand All @@ -252,19 +254,19 @@ impl crate::AuthenticatedCmd for CmdApi {
let items_core = &value_out[2..len - 2];

if comma_needed {
print!(",");
print_nopipe!(",");
}
println!();
print!("{}", items_core);
println_nopipe!();
print_nopipe!("{}", items_core);
}
Ok(true)
})
.await;
println!();
println!("]");
println_nopipe!();
println_nopipe!("]");

if let Err(e) = &result {
println!("An error occurred during a paginated query:\n{}", e);
println_nopipe!("An error occurred during a paginated query:\n{}", e);
}
result.map(|_| ())
}
Expand Down Expand Up @@ -387,7 +389,7 @@ fn print_headers(headers: &reqwest::header::HeaderMap) -> Result<()> {
tw.flush()?;

let table = String::from_utf8(tw.into_inner()?)?;
println!("{}", table);
println_nopipe!("{}", table);

Ok(())
}
Expand Down
12 changes: 7 additions & 5 deletions cli/src/cmd_auth.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use toml_edit::{Item, Table};
use uuid::Uuid;

use crate::context::Context;
use crate::{AsHost, RunnableCmd};
use crate::{println_nopipe, AsHost, RunnableCmd};

/// Login, logout, and get the status of your authentication.
///
Expand Down Expand Up @@ -441,11 +441,11 @@ impl CmdAuthStatus {
match client.current_user_view().send().await {
Ok(user) => {
log::debug!("success response for {} (env): {:?}", host_env, user);
println!("Logged in to {} as {}", host_env, user.id)
println_nopipe!("Logged in to {} as {}", host_env, user.id)
}
Err(e) => {
log::debug!("error response for {} (env): {}", host_env, e);
println!("{}: {}", host_env, Self::error_msg(&e))
println_nopipe!("{}: {}", host_env, Self::error_msg(&e))
}
};
} else {
Expand All @@ -467,9 +467,11 @@ impl CmdAuthStatus {
}
};

println!(
println_nopipe!(
"Profile \"{}\" ({}) status: {}",
profile_name, profile_info.host, status
profile_name,
profile_info.host,
status
);
}
}
Expand Down
31 changes: 18 additions & 13 deletions cli/src/cmd_disk.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

// Copyright 2024 Oxide Computer Company

use crate::{eprintln_nopipe, println_nopipe};

use anyhow::bail;
use anyhow::Result;
use async_trait::async_trait;
Expand Down Expand Up @@ -190,9 +192,10 @@ impl CmdDiskImport {
.await;

if let Err(e) = response {
eprintln!(
eprintln_nopipe!(
"trying to unwind, deleting {:?} failed with {:?}",
self.disk, e
self.disk,
e
);
return Err(e.into());
}
Expand All @@ -210,9 +213,10 @@ impl CmdDiskImport {

// If this fails, then the disk will remain in state "import-ready"
if let Err(e) = response {
eprintln!(
eprintln_nopipe!(
"trying to unwind, finalizing {:?} failed with {:?}",
self.disk, e
self.disk,
e
);
return Err(e.into());
}
Expand All @@ -231,9 +235,10 @@ impl CmdDiskImport {
// If this fails, then the disk will remain in state
// "importing-from-bulk-writes"
if let Err(e) = response {
eprintln!(
eprintln_nopipe!(
"trying to unwind, stopping the bulk write process for {:?} failed with {:?}",
self.disk, e
self.disk,
e
);
return Err(e.into());
}
Expand Down Expand Up @@ -334,7 +339,7 @@ impl crate::AuthenticatedCmd for CmdDiskImport {
.await;

if let Err(e) = start_bulk_write_response {
eprintln!("starting the bulk write process failed with {:?}", e);
eprintln_nopipe!("starting the bulk write process failed with {:?}", e);

// If this fails, the disk is in state import-ready. Finalize it so
// it can be deleted.
Expand Down Expand Up @@ -402,7 +407,7 @@ impl crate::AuthenticatedCmd for CmdDiskImport {
let n = match file.by_ref().take(CHUNK_SIZE).read_to_end(&mut chunk) {
Ok(n) => n,
Err(e) => {
eprintln!(
eprintln_nopipe!(
"reading from {} failed with {:?}",
self.path.to_string_lossy(),
e,
Expand All @@ -420,7 +425,7 @@ impl crate::AuthenticatedCmd for CmdDiskImport {
let encoded = base64::engine::general_purpose::STANDARD.encode(&chunk[0..n]);

if let Err(e) = senders[i % UPLOAD_TASKS].send((offset, encoded, n)).await {
eprintln!("sending chunk to thread failed with {:?}", e);
eprintln_nopipe!("sending chunk to thread failed with {:?}", e);
break Err(e.into());
}
} else {
Expand Down Expand Up @@ -456,7 +461,7 @@ impl crate::AuthenticatedCmd for CmdDiskImport {

if results.iter().any(|x| x.is_err()) {
// If any of the upload threads returned an error, unwind the disk.
eprintln!("one of the upload threads failed");
eprintln_nopipe!("one of the upload threads failed");
self.unwind_disk_bulk_write_stop(client).await?;
self.unwind_disk_finalize(client).await?;
self.unwind_disk_delete(client).await?;
Expand All @@ -475,7 +480,7 @@ impl crate::AuthenticatedCmd for CmdDiskImport {
.await;

if let Err(e) = stop_bulk_write_response {
eprintln!("stopping the bulk write process failed with {:?}", e);
eprintln_nopipe!("stopping the bulk write process failed with {:?}", e);

// Attempt to unwind the disk, although it will probably fail - the
// first step is to stop the bulk write process!
Expand All @@ -498,7 +503,7 @@ impl crate::AuthenticatedCmd for CmdDiskImport {
let finalize_response = request.send().await;

if let Err(e) = finalize_response {
eprintln!("finalizing the disk failed with {:?}", e);
eprintln_nopipe!("finalizing the disk failed with {:?}", e);

// Attempt to unwind the disk, although it will probably fail - the
// first step is to finalize the disk!
Expand Down Expand Up @@ -541,7 +546,7 @@ impl crate::AuthenticatedCmd for CmdDiskImport {
.await?;
}

println!("done!");
println_nopipe!("done!");

Ok(())
}
Expand Down
34 changes: 18 additions & 16 deletions cli/src/cmd_net.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ use std::io::Write;
use tabwriter::TabWriter;
use uuid::Uuid;

use crate::println_nopipe;

// We do not yet support port breakouts, but the API is phrased in terms of
// ports that can be broken out. The constant phy0 represents the first port
// in a breakout.
Expand Down Expand Up @@ -875,14 +877,14 @@ impl AuthenticatedCmd for CmdPortConfig {
.map(|x| (x.id, x.name))
.collect();

println!(
println_nopipe!(
"{}{}{}",
p.switch_location.to_string().blue(),
"/".dimmed(),
p.port_name.blue(),
);

println!(
println_nopipe!(
"{}",
"=".repeat(p.port_name.len() + p.switch_location.to_string().len() + 1)
.dimmed()
Expand All @@ -900,7 +902,7 @@ impl AuthenticatedCmd for CmdPortConfig {
writeln!(&mut tw, "{:?}\t{:?}\t{:?}", l.autoneg, l.fec, l.speed,)?;
}
tw.flush()?;
println!();
println_nopipe!();

writeln!(
&mut tw,
Expand All @@ -923,7 +925,7 @@ impl AuthenticatedCmd for CmdPortConfig {
writeln!(&mut tw, "{}\t{}\t{:?}", addr, *alb.0.name, a.vlan_id)?;
}
tw.flush()?;
println!();
println_nopipe!();

writeln!(
&mut tw,
Expand Down Expand Up @@ -987,7 +989,7 @@ impl AuthenticatedCmd for CmdPortConfig {
)?;
}
tw.flush()?;
println!();
println_nopipe!();

// Uncomment to see full payload
//println!("");
Expand Down Expand Up @@ -1017,13 +1019,13 @@ impl AuthenticatedCmd for CmdBgpStatus {
.iter()
.partition(|x| x.switch == SwitchLocation::Switch0);

println!("{}", "switch0".dimmed());
println!("{}", "=======".dimmed());
println_nopipe!("{}", "switch0".dimmed());
println_nopipe!("{}", "=======".dimmed());
show_status(&sw0)?;
println!();
println_nopipe!();

println!("{}", "switch1".dimmed());
println!("{}", "=======".dimmed());
println_nopipe!("{}", "switch1".dimmed());
println_nopipe!("{}", "=======".dimmed());
show_status(&sw1)?;

Ok(())
Expand Down Expand Up @@ -1078,12 +1080,12 @@ impl AuthenticatedCmd for CmdPortStatus {
sw0.sort_by_key(|x| x.port_name.as_str());
sw1.sort_by_key(|x| x.port_name.as_str());

println!("{}", "switch0".dimmed());
println!("{}", "=======".dimmed());
println_nopipe!("{}", "switch0".dimmed());
println_nopipe!("{}", "=======".dimmed());
self.show_switch(client, "switch0", &sw0).await?;

println!("{}", "switch1".dimmed());
println!("{}", "=======".dimmed());
println_nopipe!("{}", "switch1".dimmed());
println_nopipe!("{}", "=======".dimmed());
self.show_switch(client, "switch1", &sw1).await?;

Ok(())
Expand Down Expand Up @@ -1228,9 +1230,9 @@ impl CmdPortStatus {
}

ltw.flush()?;
println!();
println_nopipe!();
mtw.flush()?;
println!();
println_nopipe!();

Ok(())
}
Expand Down
Loading

0 comments on commit f472bce

Please sign in to comment.