dump subcommand apparently nonfunctional

[wesolows]

The dump subcommand added by 74e720c without a bug ID is undocumented and doesn't seem to work:

$ cargo run -- --help
    Finished dev [unoptimized + debuginfo] target(s) in 0.39s
     Running `target/debug/amd-host-image-builder --help`
amd-host-image-builder-generate 0.1.1

USAGE:
    amd-host-image-builder generate [FLAGS] [OPTIONS] --config <efs-configuration-filename> --output-file <output-filename> --reset-image <reset-image-filename>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information
    -v, --verbose

OPTIONS:
    -B, --blobdir <blobdirs>...
    -c, --config <efs-configuration-filename>
    -o, --output-file <output-filename>
    -r, --reset-image <reset-image-filename>

Note that there is no reference to the dump subcommand. But if one does:

$ cargo run -- dump --help
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/amd-host-image-builder dump --help`
amd-host-image-builder-dump 0.1.1

USAGE:
    amd-host-image-builder dump [OPTIONS] --existing-file <input-filename>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -b, --blob-dump-directory <blob-dump-dirname>
    -i, --existing-file <input-filename>

it becomes clear that it exists. Unfortunately, it doesn't ever seem to work, always displaying the same error message regardless of what input image it's given, including those that were generated by the tool itself:

$ cargo run -- dump -i ./milan-gimlet-b-1.0.0.a.img
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/amd-host-image-builder dump -i ./milan-gimlet-b-1.0.0.a.img`
Error: value unknown

As a general rule, the introduction of a feature (in any of our software) should have the following attributes:

1. A ticket should be filed to describe the RFE being introduced, and that ticket number should be provided in the commit message that introduces it.
2. The feature itself should be complete when delivered. Completeness varies with the scope and nature of the feature, but might include things like documentation (a man page, README, etc), help output, and passing test cases. The feature may have future improvements planned, but it should not be integrated at all until it provides a useful and coherent set of functionality. Future improvements that are planned should each have an additional ticket filed to track the RFE.
3. Test cases are optional for some features because some features are hard to test. However, for this particular tool they should be considered mandatory because the tool is idempotent and has no side effects: every possible invocation of the tool reads a well-defined set of inputs and is expected to produce the same set of well-defined outputs every time it receives the same inputs. New options or subcommands should not be introduced for utilities or library routines that are idempotent and side-effect-free without test cases to accompany them. If we lack adequate infrastructure to implement the required test cases or part of the output is architecturally considered to have a commitment level of "Not An Interface", that part should still have some description of how it was tested in the ticket so that reviewers can assess whether testing was adequate before approving. That can also be useful if the feature being introduced is purposefully not being documented, as it provides a place for users to discover how to use it.

This ticket covers filling those gaps in 74e720c.

[daym]

Absolutely. (note: 74e720c lists bug id #138 --or do you mean some other kind of bug id? Like in airtable?)

About this specific problem:

That's probably because the spi mode in the image is invalid for that processor generation. That was always iffy and is now not allowed anymore.

Please keep that image so we can examine it.

Could you try editing amd-efs v0.3.0 like so:

diff --git a/Cargo.toml b/Cargo.toml
index cf0454c..e56f4d9 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -28,6 +28,8 @@ strum_macros = { version = "0.23.1" }
 zerocopy = "0.6"
 thiserror = { version = "1.0.38", optional = true }
 memoffset = "0.5"
+# TODO disable proc-macro
+quote = "1.0.33"
 
 [features]
 default = []
diff --git a/src/serializers.rs b/src/serializers.rs
index c0bea80..94f0b7d 100644
--- a/src/serializers.rs
+++ b/src/serializers.rs
@@ -7,6 +7,7 @@
 
 use crate::ondisk::*;
 use crate::struct_accessors::DummyErrorChecks;
+use quote::quote;
 
 // Note: This is written such that it will fail if the underlying struct has fields added/removed/renamed--if those have a public setter.
 macro_rules! make_serde{($StructName:ident, $SerdeStructName:ident, [$($field_name:ident),* $(,)?]
@@ -30,7 +31,7 @@ macro_rules! make_serde{($StructName:ident, $SerdeStructName:ident, [$($field_na
                 {
                     $SerdeStructName {
                         $(
-                            $field_name: self.$field_name().map_err(|_| serde::ser::Error::custom("value unknown"))?.into(),
+                            $field_name: self.[<$field_name>]().map_err(|_| serde::ser::Error::custom(format!("textual representation of value for field '{}.{}' unknown", quote!($StructName), quote!($field_name))))?.into(),
                         )*
                 }.serialize(serializer)
             }

and then edit amd-host-image-builder like so

diff --git a/Cargo.toml b/Cargo.toml
index 62c24f1..65df4fe 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -8,7 +8,7 @@ edition = "2018"
 
 [dependencies]
 amd-apcb = { git = "https://github.com/oxidecomputer/amd-apcb.git", tag = "v0.2.0", features = ["std", "serde", "schemars"] }
-amd-efs = { git = "ssh://[email protected]/oxidecomputer/amd-efs.git", tag = "v0.3.0", features = ["std", "serde", "schemars"] }
+amd-efs = { path = "../amd-efs", features = ["std", "serde", "schemars"] }
 amd-flash = { git = "ssh://[email protected]/oxidecomputer/amd-flash.git", tag = "v0.2.1", features = ["std"] }
 goblin = { version = "0.4", features = ["elf64", "endian_fd"] }
 #serde = { version = "1.0", default-features = false, features = ["derive"] }
diff --git a/amd-host-image-builder-config/Cargo.toml b/amd-host-image-builder-config/Cargo.toml
index 429b724..b226d62 100644
--- a/amd-host-image-builder-config/Cargo.toml
+++ b/amd-host-image-builder-config/Cargo.toml
@@ -7,7 +7,7 @@ edition = "2018"
 
 [dependencies]
 amd-apcb = { git = "https://github.com/oxidecomputer/amd-apcb.git", tag = "v0.2.0", features = ["std", "serde", "schemars"] }
-amd-efs = { git = "ssh://[email protected]/oxidecomputer/amd-efs.git", tag = "v0.3.0", features = ["std", "serde", "schemars"] }
+amd-efs = { path = "../../amd-efs", features = ["std", "serde", "schemars"] }
 amd-flash = { git = "ssh://[email protected]/oxidecomputer/amd-flash.git", tag = "v0.2.1", features = ["std"] }
 schemars = "0.8.8"
 serde = { version = "1.0", default-features = false, features = ["derive"] }

(or with appropriate path to the directory of the edited amd-efs)

Then compile and run amd-host-image-builder in dump mode.

That should give you the struct and field name that caused the value unknown.

We should add that error reporting to amd-efs permanently once it does work.

[wesolows]

Here are a few images that reproduce this error; the changes in the diffs above don't compile. I'm currently at 9ccc964 with ../amd-efs at d2b1f0567d63b41bd7195a5f509d3550bad2ee2a and the above diff; I get:

...
warning: `amd-efs` (lib) generated 2 warnings
   Compiling amd-host-image-builder-config v0.1.0 (/home/wesolows/src/amd-host-image-builder/amd-host-image-builder-config)
error[E0277]: the trait bound `EfhBulldozerSpiMode: Default` is not satisfied
   --> amd-host-image-builder-config/src/lib.rs:283:28
    |
283 | #[derive(serde::Serialize, serde::Deserialize, schemars::JsonSchema)]
    |                            ^^^^^^^^^^^^^^^^^^ the trait `Default` is not implemented for `EfhBulldozerSpiMode`
    |
    = note: this error originates in the derive macro `serde::Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `EfhNaplesSpiMode: Default` is not satisfied
   --> amd-host-image-builder-config/src/lib.rs:283:28
    |
283 | #[derive(serde::Serialize, serde::Deserialize, schemars::JsonSchema)]
    |                            ^^^^^^^^^^^^^^^^^^ the trait `Default` is not implemented for `EfhNaplesSpiMode`
    |
    = note: this error originates in the derive macro `serde::Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)
... (more like these) ...

milan-ethanol-x.img.gz
milan-gimlet-b.img.gz
milan-gimlet-b-1.0.0.9.img.gz
milan-gimlet-b-1.0.0.a.img.gz

[daym]

Thanks for the images. I could reproduce the problem locally now.

As for the trait Default thing, that changed after amd-efs v0.3.0. [1] So make sure to use the amd-efs version that's listed in amd-host-image-builder's Cargo.toml as a base to edit (so git checkout v0.3.0 inside amd-efs).

With that and your image milan-ethanol-x.img.gz (and your other images, too), dump says:

Error: textual representation of value for field 'EfhRomeSpiMode.fast_speed_new' unknown

I'll think about whether it's possible without too many downsides to still make that dump, but just leave the spi mode off in the resulting file. But with #157 the error message is already a lot better (... for that special case). But changing src/serializers.rs like we do here makes it better for all fields, so what we are doing here would still be very useful.

[1] This main can be ahead is a compromise so we can have cross repo PRs where the amd-efs one was merged already but the amd-host-image-builder one wasn't merged yet and have the regular build of amd-host-image-builder still work (... with the previous amd-efs).

[wesolows]

More generally, we probably want to be able to dump even if some fields can't be properly decoded. FWIW, these were generated recently but apparently without the change made in oxidecomputer/helios#74 (which doesn't seem to have been propagated to the examples). I can obviously fix that in the image, but if we want to be able to use the dump feature for its most important use case -- exploring unknown attributes of images from new processor families -- it's probably going to be necessary.

[wesolows]

Absolutely. (note: 74e720c lists bug id #138 --or do you mean some other kind of bug id? Like in airtable?)

Sorry. I was looking for the convention I'm used to, the one we use in all the illumos repos, and overlooked the separate line with a link to the issue, which I think I confused with something produced by git. Or something. At some point we should probably adopt the same convention as is used elsewhere, but that's not a reason I couldn't have found the ticket in this instance.