RFC: guide: document saved state conventions and howto

[mattkur]

Add some documentation on how devs should author and think about saved state.

[yupavlen-ms]

As a follow-up we need a reference to SaveRestore trait, how it applies to vmcore crate and why some drivers may not be able to use it.

[jstarks]

Please wrap all markdown at 80 chars for easier offline reading. Consider the "rewrap" extension in vscode.

[jstarks]

Can you add a section about how to extend an existing saved state blob? Notes:

• When you read an old saved state on a new build, any fields that were missing will get their default value.
  • Option<T> => None,
  • Numbers => 0,
  • Strings => "",
  • Structs => each field gets its default value
  • Enums => currently fails: if you need to add an enum, add it as Option<MyEnum>
  • Vecs => empty vec
  • Arrays => fails: if you need to add an array, consider a Vec or Option<[T; N]>.
• When you read a new saved state on an old build, unknown fields will be ignored.

[jstarks]

I've opened #848 to address the problem with adding a new enum field causing a potential compat break.

[mattkur]

As a follow-up we need a reference to SaveRestore trait, how it applies to vmcore crate and why some drivers may not be able to use it.

Thanks for calling this out. I'm happy to add that to this PR, but haven't done the research to answer the questions raised here. Alternatively, please file an issue so we don't forget.

[jstarks]

They are supported, you just need to wrap them in Option.

[jstarks]

Feel free to choose a stopping point for this PR. But I would also want to provide some more concrete advice, such as:

When a field has been added and your code reads an old saved state that did not have that field, the field will get the default value, as defined below. You must define the field's type so that the default value makes sense when reading an old saved state.

The easiest way to do this is to add fields with a type Option<T>, so that you can distinguish between old saved states (None) and new ones (Some(_)). But for field types with well-defined default values, this just adds an extra layer of complexity, so consider defining things so that the default value means the same thing as the value being missing.

For example, if you are saving whether the guest enabled some new feature, just use a bool for that, since false is a reasonable value for an old saved state where the feature didn't exist; Option<bool> just adds a third state that the reader has to go look in your save/restore code to understand.

However, for types where there are no default values (currently, enums and arrays), you must wrap the type in an Option to add it to an existing saved state structure.

[daprilik]

For the bool case, that makes a lot of sense. On the flip-side, I'm not sure how I'd feed about seeing something like queue_count: usize (or whatever), where 0 is the in-band representation of missing state.

In general, the Go/Protobuf style of using defaults as in-band signals for missing data just rubs me the wrong way, as now the compiler isn't gonna force you to consider those cases explicitly.

[jstarks]

If queue_count would otherwise never be 0 in a newly generated saved state, then I agree. E.g., if the natural default for queue count were 1, then I wouldn't want us to treat 0 the same as 1--we should use an Option to clarify that the value might be missing (ideally with NonZeroU32 or whatever, which I don't think we support today but we could).

Maybe another way to phrase it is, if you have to have a conditional (explicitly or implicitly via unwrap_or_else or similar) in your restore path to handle the missing case, it should probably be on an Option. If you wouldn't have needed the condition except that you chose to use an Option... well, maybe that was the wrong choice.

Most egregious to me is where the newly added field has a natural default of true, so that None is equivalent to Some(true) rather than Some(false). The guidance should be to flip the parity of the bool in that case, rather than have this unintuitive mapping.

[mattkur]

I'd like to hit pause on new content for this PR. I'll fix up the comment above (#846 (comment)). When I hit pause, I'll file an issue to track any feedback not yet captured.