search

oxidecomputer / amd-apcb

AMD Generic Encapsulated Software Architecture Platform Security Processor Configuration Block manipulation library
Mozilla Public License 2.0
13 stars 1 forks source link

apcb: When deserializing Apcb, ignore user-specified apcb_size. #43

Closed daym closed 2 years ago

daym commented 2 years ago

Previously, the buffer into which the Apcb was deserialized was fixed size and was sized using apcb_size that the user specified (apcb_size is supposed to actual used size of the apcb, think size as opposed to capacity). This was not the intention and is user-unfriendly.

Therefore, this changes it such that the buffer into which the Apcb is deserialized has maximal size. The user-specified apcb_size is ignored, and instead the value of apcb_size to go into the Apcb header is calculated by the size of the things that go into the body.

This fixes amd-host-image-builder issue #55.

daym commented 2 years ago

Also found and fixed https://github.com/oxidecomputer/amd-apcb/issues/45.

daym commented 2 years ago

Are there tests that can be added for this?

Definitely! I will add some tests.

For example, do we have unit test cases for V2-only and V3 headers?

Nope. Sorely missing. That is how the bug made it this long.

Since v3_header_ext is new and v3_header is no longer used (right?), is there a new test case for that? There should be a regression test case for this bug too.

The problem was that in one case, a field was called v3_header, and in another case, the same field was called v3_header_ext, making it not match the configuration file body at runtime. This specific field is optional, which meant it defaulted (... to a useful default that made it boot).

Because this PR here, more specifically the commit "apcb: Automatically infer value for header_size on deserialization." detects whether v3_header_ext is present in the configuration or not, and sets header_size accordingly, NOW we had made it a visible problem... (previously, header_size had been specified by the user--presumably correctly)

In view of existing configuration files--which contain v3_header_ext--I fixed it to always say v3_header_ext.

daym commented 2 years ago

Added tests.