Improve dump subcommand. #162

daym · 2023-09-21T22:24:06Z

Fixes #160.

Fixes <#160>.

daym · 2023-11-28T00:37:35Z

Depends on oxidecomputer/amd-apcb#115 and will not build until that one is merged.

wesolows

The dummy dump at the end is a good idea. I didn't look at the serde stuff because it's beyond my feeble mind.

wesolows · 2024-01-23T17:51:40Z

Cargo.toml

+amd-apcb = { git = "https://github.com/oxidecomputer/amd-apcb.git", branch = "issue-113", features = ["std", "serde", "schemars"] }
+amd-efs = { git = "ssh://[email protected]/oxidecomputer/amd-efs.git", branch = "issue-99", features = ["std", "serde", "schemars"] }


I don't think this is intended?

It's so the github CI and thus buildomat can build it. Otherwise all we'd see is a build failure.

But yeah, definitely not merge it like that.

wesolows · 2024-01-23T17:51:50Z

amd-host-image-builder-config/Cargo.toml

+amd-apcb = { git = "https://github.com/oxidecomputer/amd-apcb.git", branch = "issue-113", features = ["std", "serde", "schemars"] }
+amd-efs = { git = "ssh://[email protected]/oxidecomputer/amd-efs.git", branch = "issue-99", features = ["std", "serde", "schemars"] }


wesolows · 2024-01-23T17:55:05Z

src/main.rs

@@ -735,25 +772,48 @@ fn dump(
        if filesize <= 0x100_0000 { Some(filesize as u32) } else { None };
    let efs = Efs::load(&storage, None, amd_physical_mode_mmio_size).unwrap();
    if !efs.compatible_with_processor_generation(ProcessorGeneration::Milan) {
-        panic!("only Milan is supported for dumping right now");
+        if !efs.compatible_with_processor_generation(ProcessorGeneration::Rome)


Surely this could be phrased as a compound condition. Is there a reason not to?

dancrossnyc · 2024-01-23T18:02:15Z

src/main.rs

+    fallback: T,
+    result_text: &str,
+) -> T {
+    match result {


This is unwrap_or_else, isn't it? E.g.,

result.unwrap_or_else(|e| { eprintln!("{result_text} was invalid: {e}. Falling back to default"); default })

dancrossnyc · 2024-01-23T18:06:00Z

src/serializers.rs

+impl std::fmt::Display for PathNode {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        match self.next {
+            Some(ref x) => {


I would phrase this as an if let. E.g.,

if let Some(ref x) = self.next { x.fmt(f)?; write!(f, "/")?; }

dancrossnyc · 2024-01-23T18:08:23Z

src/serializers.rs

+    }
+}
+
+type Ok = ();


How/where is this used?

Most of the serializer impls use it (type Ok = Ok), for example impl<'a> ser::Serializer for &'a mut DummySerializer in line 238.

I didn't want to repeat the concrete respective Ok and Error types in each of the ~20 impls, so I introduced global type aliases Ok and Error.

Since the custom serializer only exists to find errors (and not serialize into anything), Ok = ().

Perhaps then use it on lines 70, 88, 104, 124, 149, 193, and 216 as well? And any other places that I missed?

Indeed!

Changed.

Some background: I wasn't sure whether I'd have to thread through the error infos (path and raw value of unknown thing in zerocopy struct) in the Ok variant--so this is why this is parametrizable in the first place.

dancrossnyc · 2024-01-23T19:12:03Z

src/serializers.rs

+    fn skip_field(&mut self, key: &'static str) -> Result<(), Self::Error> {
+        eprintln!(
+            "{}: skipped field {}",
+            match self.path {


This is unwrap_or_default, I think. You may need a map first. e.g., self.path.map(|p| p.to_string()).unwrap_or_default().

dancrossnyc · 2024-01-23T19:14:31Z

src/serializers.rs

+        T: Serialize,
+    {
+        value.serialize(&mut DummySerializer {
+            path: self.path.clone(), // FIXME check


What's the comment refer to?

In general, this is doing path building inside the virtual json (or whatever it is it's serializing to). The path to the json node is then printed on error.

All those path: self.path.clone() // FIXME is supposed to allude to that this specific combination wasn't tested yet.

For example, if you are in DummySerializer and do serialize_seq, should that introduce a path element (representing the entire seq)? Probably not, but I've learned to check. Also, I'm not sure whether all Rust upstream serializers do it the same way. I think serde json does some flattening--and I guess we should just adapt to what serde json does, even within our dummy serializer.

Likewise, SerializeSeq's serialize_element in SerializeVec could add a vec element index to the path, but currently it does not. Should it? Probably.

dancrossnyc · 2024-01-23T19:15:05Z

src/serializers.rs

+}
+
+impl<'a> ser::SerializeStruct for &'a mut DummySerializer {
+    // Must match the `Ok` type of the serializer.


This is clear, and these comments don't show up any other places similar types are defined.

dancrossnyc · 2024-01-23T19:15:31Z

src/serializers.rs

+    fn skip_field(&mut self, key: &'static str) -> Result<(), Self::Error> {
+        eprintln!(
+            "{}: skipped field {}",
+            match self.path {


See above for replacing the match with some combination of map and unwrap_or_default

dancrossnyc · 2024-01-23T19:15:41Z

src/serializers.rs

+    fn skip_field(&mut self, key: &'static str) -> Result<(), Self::Error> {
+        eprintln!(
+            "{}: skipped field {}",
+            match self.path {


daym force-pushed the issue-160 branch 2 times, most recently from e679734 to 408e711 Compare November 14, 2023 17:42

Improve dump subcommand.

5f9cc71

Fixes <#160>.

daym force-pushed the issue-160 branch from 408e711 to 5f9cc71 Compare November 14, 2023 17:45

daym force-pushed the issue-160 branch 4 times, most recently from 353fbe6 to 482151e Compare November 28, 2023 00:36

daym force-pushed the issue-160 branch 7 times, most recently from 013d690 to a8e64fe Compare November 28, 2023 00:48

daym requested a review from wesolows November 28, 2023 00:49

daym force-pushed the issue-160 branch from a8e64fe to 06e6936 Compare November 28, 2023 00:58

wesolows reviewed Jan 23, 2024

View reviewed changes

daym requested a review from dancrossnyc January 23, 2024 18:00

dancrossnyc reviewed Jan 23, 2024

View reviewed changes

Use Permissive Serializers in order to collect errors without fail.

8ea7533

daym force-pushed the issue-160 branch from 06e6936 to 8ea7533 Compare January 23, 2024 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve dump subcommand. #162

Improve dump subcommand. #162

daym commented Sep 21, 2023

daym commented Nov 28, 2023 •

edited

Loading

wesolows left a comment

wesolows Jan 23, 2024

daym Jan 23, 2024 •

edited

Loading

wesolows Jan 23, 2024

wesolows Jan 23, 2024

dancrossnyc Jan 23, 2024 •

edited

Loading

dancrossnyc Jan 23, 2024

dancrossnyc Jan 23, 2024

daym Jan 23, 2024 •

edited

Loading

dancrossnyc Jan 23, 2024

daym Jan 23, 2024 •

edited

Loading

dancrossnyc Jan 23, 2024 •

edited

Loading

dancrossnyc Jan 23, 2024

daym Jan 23, 2024 •

edited

Loading

dancrossnyc Jan 23, 2024

dancrossnyc Jan 23, 2024

dancrossnyc Jan 23, 2024

		amd-apcb = { git = "https://github.com/oxidecomputer/amd-apcb.git", branch = "issue-113", features = ["std", "serde", "schemars"] }
		amd-efs = { git = "ssh://[email protected]/oxidecomputer/amd-efs.git", branch = "issue-99", features = ["std", "serde", "schemars"] }

Improve dump subcommand. #162

Are you sure you want to change the base?

Improve dump subcommand. #162

Conversation

daym commented Sep 21, 2023

daym commented Nov 28, 2023 • edited Loading

wesolows left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daym Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dancrossnyc Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daym Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daym Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

dancrossnyc Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daym Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daym commented Nov 28, 2023 •

edited

Loading

daym Jan 23, 2024 •

edited

Loading

dancrossnyc Jan 23, 2024 •

edited

Loading

daym Jan 23, 2024 •

edited

Loading

daym Jan 23, 2024 •

edited

Loading

dancrossnyc Jan 23, 2024 •

edited

Loading

daym Jan 23, 2024 •

edited

Loading