Add library for `humility dump` by labbott · Pull Request #698 · oxidecomputer/humility

labbott · 2026-06-10T16:09:45Z

This is broken up into two commits

Refactor humility dump to make some of the logic easier to follow
Add the dumping library

Because of the way humility dump currently works it's tricky to have all the library functions replace the same functions in the binary. Almost tempted to say we should start from scratch with humility dump-ng but that's probably a bad idea.

This function has grown complicated and hard to follow. Extract the following to separate functions - `subargs.initialize_dump_agent` - `subargs.simulate_dumper`/`subargs.emulate_dumper`/ `subargs.simulate_task_dump` - `subargs.extract`/`subargs.force_read` There's a little bit of redundancy but it makes the logic much easier to follow.

One of the most common uses for humility is to take an SP dump on a system so it can be analyzed later. Having this as a library function makes it easier to do it more places.

mkeeter · 2026-06-16T15:00:11Z

@@ -0,0 +1,20 @@
+[package]
+name = "humility-dump-lib"


Nit: I would expect this to just be humility-dump for consistency with other library crates (same with folder name).

I did humility-vpd-lib before but I'm also not attached to the -lib suffix if others prefer it be dropped 🤷

Yeah, dropping the lib is my preference (sorry I didn't notice in humility-vpd-lib)

mkeeter · 2026-06-16T15:00:45Z

+version = "0.1.0"
+edition.workspace = true
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html


probably don't need this

Suggested change

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

mkeeter · 2026-06-16T15:19:43Z

+pub enum DumpError {
+    /// Tried to take a dump when there were pending dumps. The
+    /// dumps should be extracted first before trying to take a dump.
+    #[error("Cannot take a dump while one is already in progress")]


Nit: error Display strings should be lower case, per C-GOOD-ERR

mkeeter · 2026-06-16T15:20:18Z

+                .map_err(DumpError::UdpAgent)?,
+        ))
+    } else {
+        // XXX


I think this was a reminder that I grabbed a timeout instead of having it passed in. I'm also looking at the code and timeout doesn't get used anywhere except humility-net-core which is the exact opposite case here! I might take a pass to fix this up.

mkeeter · 2026-06-16T15:23:15Z

+                    humpty::DUMP_CONTENTS_TASKREGION => {
+                        format!("{}.region", module.name.to_owned())
+                    }
+                    c => panic!("unknown contents type: {c}"),


Return an error instead here?

mkeeter · 2026-06-16T15:27:29Z

+}
+
+/// Take and collect a dump via the agent. If a file is not
+/// specified the file will be saved accoridng to the default


Suggested change

/// specified the file will be saved accoridng to the default

/// specified the file will be saved according to the default

mkeeter · 2026-06-16T15:30:10Z

+pub fn take_system_dump_via_agent(
+    hubris: &HubrisArchive,
+    core: &mut dyn Core,
+    dumpfile: Option<&PathBuf>,


I don't love having the naming logic in the library (I would be happier if this took a std::io::Write handle), but that's annoying to fix right now since it's actually in HubrisArchive::dump.

mkeeter · 2026-06-16T15:30:34Z

+    None,
+    /// A whole system dump and its size
+    System(u32),
+    /// Individual hubris tasks


Suggested change

/// Individual hubris tasks

/// Individual Hubris tasks

mkeeter · 2026-06-16T15:34:16Z

+    let headers =
+        agent.read_dump_headers(false).map_err(DumpError::DumpHeaders)?;
+
+    if headers.is_empty() || headers[0].0.dumper == humpty::DUMPER_NONE {


Nit: because we pass raw: false to read_dump_headers, is it even possible to get DUMPER_NONE?

("This is how it worked before and I don't want to deal with it in this PR" is a valid response here)

Yeah I was copy/pasting the existing logic. I think you are correct but I'm lightly wary to remove it without more testing. I might just add another comment.

mkeeter · 2026-06-16T15:40:34Z

+                    humpty::DUMP_CONTENTS_TASKREGION => {
+                        format!("{} [region]", module.name.to_owned())
+                    }
+                    c => panic!("unknown contents type: {c}"),


Return error instead of panicking?

mkeeter · 2026-06-16T15:41:09Z

+        tasks.push(TaskDumpData {
+            area: *area,
+            size,
+            task_name,


Do we want a richer task name here in the API, instead of baking it into a string immediately?

can you say more? It sounds like you may be proposing returning a enum ?

Yeah, replacing task_name: String with task_name: TaskName, with

pub enum TaskDumpName { SingleTask(String), TaskRegion(String), Unknown, } impl std::fmt::Display for TaskDumpName { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::SingleTask(s) => s.fmt(f), Self::TaskRegion(s) => write!(f, "{s} [region]"), Self::Unknown => "<unknown>".fmt(f), } } }

mkeeter · 2026-06-16T15:42:54Z

+                _ => "<unknown>".to_owned(),
+            };
+
+        let dumpfile = (0..)


You can skip this and just pass None to hubris.dump(..); it will then do this same logic internally.

Hmmmm, this wouldn't handle DUMP_CONTENTS_TASKREGION separately. I'm not sure if we ever actually want to save a TaskRegion dump to file, though?

mkeeter · 2026-06-16T15:45:10Z

+    if let Err(err) = agent.take_dump() {
+        if let Ok(all) = agent.read_dump_headers(true) {
+            let c = all
+                .iter()
+                .filter(|&&(h, _)| h.dumper != humpty::DUMPER_NONE)
+                .count();
+            if c == all.len() {
+                return Err(DumpError::DumpSpaceExhaustion(err));
+            } else if c != 0 {
+                return Err(DumpError::IncompleteDump(err));
+            } else {
+                return Err(DumpError::TakeDump(err));
+            }
+        } else {
+            return Err(DumpError::TakeDump(err));
+        }
+    }


Nit: because this is a transformation on err and always returns, I would write it as follows

Suggested change

if let Err(err) = agent.take_dump() {

if let Ok(all) = agent.read_dump_headers(true) {

let c = all

.iter()

.filter(|&&(h, _)| h.dumper != humpty::DUMPER_NONE)

.count();

if c == all.len() {

return Err(DumpError::DumpSpaceExhaustion(err));

} else if c != 0 {

return Err(DumpError::IncompleteDump(err));

} else {

return Err(DumpError::TakeDump(err));

}

} else {

return Err(DumpError::TakeDump(err));

}

}

agent.take_dump().map_err(|err| {

if let Ok(all) = agent.read_dump_headers(true) {

let c = all

.iter()

.filter(|&&(h, _)| h.dumper != humpty::DUMPER_NONE)

.count();

if c == all.len() {

DumpError::DumpSpaceExhaustion(err)

} else if c != 0 {

DumpError::IncompleteDump(err)

} else {

DumpError::TakeDump(err)

}

} else {

DumpError::TakeDump(err)

}

})?;

mkeeter · 2026-06-16T15:49:36Z

+    /// No dumps
+    None,
+    /// A whole system dump and its size
+    System(u32),


Nit: make this a named System { size: u32 }, to make it a little more obvious?

mkeeter · 2026-06-16T15:54:38Z

+/// Take and collect a dump via the agent. If a file is not
+/// specified the file will be saved accoridng to the default
+/// humility logic (hubris.core.0, hubris.core.1 etc.)
+pub fn take_system_dump_via_agent(


Rename to take_system_dump? I'm not sure what via_agent adds at this point.

I had a thought about differentiating between dump via agent vs calling hubris.dump directly. hubris.dump doesn't really need a library except for maybe a wrapper to call core.halt. I'll rename.

mkeeter · 2026-06-16T16:24:22Z

+}
+
+/// Extract an existing system dump via the agent
+pub fn extract_system_dump_via_agent(


Vibes: how about splitting this into a public and private function, e.g.

/// Extract an existing system dump pub fn extract_system_dump( hubris: &HubrisArchive, core: &mut dyn Core, dumpfile: PathBuf, use_hiffy: bool, log: &Logger, ) -> Result<(), DumpError> { let started = Instant::now(); let mut agent = get_dump_agent(hubris, core, use_hiffy, log)?; extract_system_dump_via_agent( hubris, &mut *agent, Some(&dumpfile), started, log, ) } fn extract_system_dump_via_agent( hubris: &HubrisArchive, mut agent: &mut dyn DumpAgent, dumpfile: Option<&std::path::Path>, started: std::time::Instant, log: &Logger, ) -> Result<(), DumpError> { let mut out = DumpAgentCore::new( HubrisFlashMap::new(hubris).map_err(DumpError::FlashMap)?, ); let _ = agent .read_dump(None, &mut out, false, log) .map_err(DumpError::ReadDump)?; hubris .dump(&mut out, None, dumpfile, Some(started), log) .map_err(DumpError::HubrisDump)?; Ok(()) }

The advantage of this approach is that take_system_dump below can call extract_system_dump_via_agent, removing some duplicate logic.

labbott force-pushed the humility-dump-lib branch from ae08967 to 9fb35b1 Compare June 12, 2026 18:07

Introduce humility-dump-lib

863bb7d

One of the most common uses for humility is to take an SP dump on a system so it can be analyzed later. Having this as a library function makes it easier to do it more places.

labbott force-pushed the humility-dump-lib branch from 9fb35b1 to 863bb7d Compare June 12, 2026 18:12

mkeeter self-requested a review June 16, 2026 14:56

mkeeter reviewed Jun 16, 2026

View reviewed changes

	/// specified the file will be saved accoridng to the default
	/// specified the file will be saved according to the default

Conversation

labbott commented Jun 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants