Add `NullStr` type by jamesmunns · Pull Request #496 · oxidecomputer/management-gateway-service

jamesmunns · 2026-06-01T11:16:57Z

This is a private type that is intended to be shared across wire types that act as a null-padded fixed length container format.

This is intended to be used in a follow up PR for supporting oxidecomputer/hubris#2463, but this is separated to make reviewing this helper easier. Split out of https://github.com/oxidecomputer/management-gateway-service/tree/james/pmbus-please.

jamesmunns · 2026-06-01T16:16:36Z

There's a world where it might be better to make the wrappers even thinner, to reduce some duplicated code. e.g. something like:

pub struct SpComponent(nullstr::NullStr);

and impl deref/derefmut, but that's also Not Exactly The Best Approach.

OR, we could make the inner field public, and just use methods directly, but this exposes NullStr as part of our public interface.

OR, we could write a little decl macro that impls all the pass-throughs per-type.

All options are imperfect, open to thoughts.

hawkw · 2026-06-01T16:31:29Z

Some part of me would still like to see this replaced with the fixedstr crate that's currently used in Hubris, though perhaps the implementation has diverged enough that this doesn't make sense. Doing so would require factoring out fixedstr into its own repo so that we don't have cyclic Git deps between the hubris and management-gateway-service repos.

I'm fine with merging the two types later though, and I'm happy to merge forwards with this change for now since I know other stuff you're working on depends on it.

jamesmunns · 2026-06-01T16:35:06Z

I'm fine with merging the two types later though

I think keeping NullStr private like I have makes this much more tenable! I haven't re-looked at the fixedstr crate, but yeah, when I glanced last week it seemed to not be a 1:1 match. I'll take another look now though!

Let me know if you have any opinions on the APIs, or how I've done the plumbing with the shim layers.

Arguably, making .id private doesn't make a ton of sense, but it actually was more to hide the type, more than for enforcing invariants.

hawkw

Thanks for doing this! As I mentioned, it would be nice if, eventually, this could be merged with FixedStr, but it seems good for now.

I left some relatively minor notes; overall, the implementation looks solid.

hawkw · 2026-06-01T16:35:15Z

+        /// fail (i.e., we're always storing contents as human-readable
+        /// strings), but because we reconstitute components from network messages
+        /// we still need to check.
+        pub fn as_str(&self) -> Option<&str> {


i feel like this could return the str::FromUtf8Error rather than an Option?

It could! But this code was already there. I could change it if you think that would be a good idea though!

🤷‍♀️

hawkw · 2026-06-01T16:42:14Z

+    /// Create an [`SpComponent`] from the given slice.
+    ///
+    /// This function has some Interesting Details:
+    ///
+    /// 1. If `src` exceeds the length of `N`, only the first `N` bytes will be copied in
+    /// 2. If `src` contains null bytes, these will be copied in
+    ///
+    /// You might ask yourself, why isn't this unsafe? Why doesn't it violated any invariants?
+    /// Well, we implement `Deserialize`, which means we might obtain garbage off the wire
+    /// anyway! So, morally, this is not particularly worse to support.
+    ///
+    /// This is *primarily* intended to be done when re-magicking from a code-generated string.
+    #[inline]
+    pub fn from_bstr_unchecked(src: &[u8]) -> Self {
+        Self { id: nullstr::NullStr::from_bstr_unchecked(src) }
+    }
+
+    #[inline]
+    pub const fn from_const(val: &str) -> Self {
+        Self { id: nullstr::NullStr::from_const(val) }
+    }


huh, this all ends up a bit boilerplate-y; I thought about saying "can't we have some kinda From<T> where T: Into<NullStr>, but I guess we can't, because the whole point of the different constructors is to have some be const-callable so it doesn't really work out. This still seem sworth it, just a bit of a bother.

hawkw · 2026-06-01T16:49:03Z

I'm fine with merging the two types later though

I think keeping NullStr private like I have makes this much more tenable! I haven't re-looked at the fixedstr crate, but yeah, when I glanced last week it seemed to not be a 1:1 match. I'll take another look now though!

Yeah, it's not --- I think it may be worth a longer term project to unify the two. We could easily make FixedStr's implementation more like NullStr in several ways, as long as its CBOR encoding stuff doesn't change; that's currently the only thing there that's load-bearing.

The reason I would want this in the long term is that we use device refdes produced by codegen for both the SpComponent IDs in control-plane-agent and as device ID fields in ereports; right now, we are using FixedStr to represent them when they get CBOR'd in ereports and have to do some extra type conversion. It would be really nice if, eventually, we could just get the build-i2c codegen to generate the same type that's used in both ereports and gateway-messages. But I don't mean to make that a blocker for getting this through.

This is a private type that is intended to be shared across wire types that act as a null-padded fixed length container format. This is intended to be used in a follow up PR for supporting oxidecomputer/hubris#2463, but this is separated to make reviewing this helper easier.

hawkw

okay, this seems good to me! it might be worth testing that a faux-mgs built from this branch can happily call into the inventory and component-details APIs on a SP running the current release Hubris image, to ensure we've not accidentally broken wire compatibility someplace? i feel like that's unlikely based on the change but it is probably still worth checking.

jamesmunns · 2026-06-01T18:00:36Z

Happy to do so! Will probably be tomorrow AM my time. Appreciate the review!

jamesmunns · 2026-06-02T13:43:13Z

@hawkw tested successfully (full info in chat) with faux-mgs on this version:

read-component-caboose, exercising sending an SpComponent TO the SP as part of MgsRequest::ReadComponentCaboose (SP's GITC was observed)
uart-attach, passthru exercises SpComponent FROM the SP as part of SpRequest::SerialConsole, in the form of UART stream data (which was observed)

Codegen impl changes necessary to support oxidecomputer/management-gateway-service#496.

jamesmunns requested a review from hawkw June 1, 2026 11:16

jamesmunns force-pushed the james/nullstr-pt1 branch from 17cf992 to d05ab86 Compare June 1, 2026 11:28

jamesmunns mentioned this pull request Jun 1, 2026

Add PMBus status retrieval to MGS API #497

Draft

jamesmunns force-pushed the james/nullstr-pt1 branch from 8c134f7 to 468144f Compare June 1, 2026 16:06

jamesmunns mentioned this pull request Jun 1, 2026

Use new NullStr based SpComponent oxidecomputer/hubris#2545

Merged

hawkw reviewed Jun 1, 2026

View reviewed changes

jamesmunns force-pushed the james/nullstr-pt1 branch from 468144f to 604614c Compare June 1, 2026 16:59

jamesmunns force-pushed the james/nullstr-pt1 branch from 604614c to d06ea64 Compare June 1, 2026 17:17

hawkw approved these changes Jun 1, 2026

View reviewed changes

jamesmunns merged commit 9dd63c0 into main Jun 2, 2026
12 checks passed

jamesmunns deleted the james/nullstr-pt1 branch June 2, 2026 13:45

jamesmunns added a commit to oxidecomputer/hubris that referenced this pull request Jun 3, 2026

Use new NullStr based SpComponent (#2545)

8f5e0f7

Codegen impl changes necessary to support oxidecomputer/management-gateway-service#496.

Conversation

jamesmunns commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesmunns commented Jun 1, 2026

Uh oh!

hawkw commented Jun 1, 2026

Uh oh!

jamesmunns commented Jun 1, 2026

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hawkw Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

jamesmunns Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

hawkw Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hawkw commented Jun 1, 2026

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

jamesmunns commented Jun 1, 2026

Uh oh!

jamesmunns commented Jun 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jamesmunns commented Jun 1, 2026 •

edited

Loading