Deciding on Some Changes to Code Organization

[trent-reed]

Hello Everyone,

Nice to meet you all, my name is Trent! I work at Google on various security related things, and I'll be spending a lot of time on TPM-RS in the coming months. Emily has transitioned her leadership of the project to me, and as such I've been spending the past few months wrapping up my other work so that I can focus on TPM-RS.

This discussion isn't just to introduce myself though, I've been busy taking an inventory of this project and the direction it's heading. Over the past quarter I've learned a lot about TPMs by reading the specifications and introducing myself to the codebase. And over the past few weeks of July I've started pulling at threads relating to questions I had about the current code.

I've stepped away with a few thoughts I'd like to share on the direction I think this project should head. I'm hoping to get a gut check from others on this project if this is okay and if so we can start making some of these changes.

A Strong Foundation

I see that we're already adding some commands to the repo, and that's great - BUT I don't feel like the foundational elements of the code are very well-defined yet. I'd like for our focus to stay away from adding new commands for the time being, and towards cleaning up what we have (and adding tests, I think there's a lot of testing lacking).

I have tried a lot (but not all) of the things that I'm about to mention in my own fork. I don't intend on doing anything with that fork repo, it's just for demonstration, but if you want you can peruse it to see how some of my suggestions would pan out: https://github.com/trent-reed/tpm-rs/tree/dev/trent-reed/restructuring

Code Structure

Avoid Nested Crates

Rust has a flat crate structure, I think having crates defined within crates for the structure of the repo is kind of confusing. It makes it hard for newcomers to see what crates this project even owns. Not only that but it lacks clear delineation from one crate to the other, which feels to me like an anti-pattern.

I suggest that we bundle all the crates by their full crate name in a single directory (whether or not that's the root directory or a subdirectory named crates makes no difference to me, though since we want documentation to travel with the code I'm not sure we need any other directories; my first suggestion would be the root of the repo).

Example: https://github.com/trent-reed/tpm-rs/tree/dev/trent-reed/restructuring

Each Crate Must Have a README

This is for newbies to the project to feel more welcome. I'll admit, I had a very, very hard time understanding where everything was and what everything was for. A simple readme in the root of a crate can help with that (as well as these other code organization strategies; looking at one big lib.rs is daunting and difficult to make sense of IMO).

Example: https://github.com/trent-reed/tpm-rs/tree/dev/trent-reed/restructuring/tpm2-rs-marshal

Put Code in Separate Files

Today, most of the code that we have is in one single file (lib.rs or equivalent).

Rust has a fairly comprehensive module system that allows us to move code around. There are two aspects to this module system: What the dev sees, and what the client (using the library) sees. We are able to provide additional segmentation of source files without introducing segmentation of access to types for the client.

This is called "re-exporting":

mod some_module;  // Defined in a separate file (private, don't allow client to use).
pub use some_module::*;  // Re-Export all public types in current module.

We should use re-exporting to segment our code for better type isolation, and to allow for an easier time developing and maintaining.

Example: https://github.com/trent-reed/tpm-rs/tree/dev/trent-reed/restructuring/tpm2-rs-base/src/types

Justification

This isn't just for code organization, it's also for controlling access to functions and data. The module system and the visibility system are closely related to one-another. Any type defined in a module file can access the private functions or data of other types within the same module. Similarly, submodules can access the private contents of their parents.

This leads me to my next point...

(Most) Types Should Exist In Their Own Modules

This is for a few reasons, one is better organization, the other is better control over visibility (as stated above). There can be instances where some types need helper types, or the types have very simple definitions. So this is less of a hard restriction and more like a good starting place.

If there is a closely-related type defined in the same module and it starts to become complicated (lots of impls, tests, etc), then we should consider splitting it into a submodule. Submodules are a natural way to extend functionality and limit access to the child types while still having private access from the child to the parent.

Only Modules We Want the Client To See Should be Pub

If you want to introduce a module that the client must access directly, you simply do not (and should not) re-export the types from the module. You should just mark the module as pub:

// Accessible to the client, and you must path through this to get these types.
// Note: No re-exporting; this way there's only one path to the type for the client.
pub mod commands;

Example: https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-base/src/lib.rs

Large Tests Should Be Written in a Separate File

Rust has no such requirement that tests have to be defined in the same file as the implementation, and I don't see a point to doing it. It just complicates things by making the implementation messier. If the tests are small, or there are only a few of them and the implementation of the type is simple, maybe it's fine to define the tests in the same file - but in general err towards creating a new file for tests.

Example: https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-client/src/client/tests.rs

Avoid use super::* Outside of Tests (But We Can Temporarily Use it While Breaking Up the Codebase)

I think that for a first step towards splitting the repo into more reasonable submodules may involve using use super::*. This is roughly equivalent to the current setup of everything being in lib.rs without taking a ton of time to restructure each file to re-state all of our use statements. We should not do this in the future, but we can use it to start breaking up the code and then occasionally take cleanup PRs to slowly remove this later.

As stated, it's reasonable for tests, because you often want to use all of the things in the parent module that you want to test over. Adding explicit using statements there can be kind of annoying (it's fine if you want to though, I think; I just don't think we need to require it here).

I only mention it because my example restructuring uses it heavily.

Likewise, Probably Worth Avoiding Custom Preludes

You can simulate a custom prelude via a combination of use super::* and pub(crate) use custom_prelude::*. You'll see I do this in my example restructuring of the project. I don't actually think this is a good thing, but again like mentioned, it may be a good first step to help us break the code up without causing a lot of work for whoever splits things up.

https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-base/src/prelude.rs

I only mention it because my example restructuring uses it heavily.

Split Logic Into Separate Modules As Well

Just because we don't define different types, doesn't mean that we can't define different modules to contain different logic. If we have some function that calls other functions and the codepath is obviously separatable, then it may be reasonable to do this. Only do this when it aids in readability and/or code separation.

One such example is in the marshalable proc-macro. There's two different implementations basically - enum and struct, so it's a perfect place to split them up. This allows it to be much easier to find the code you want. And we can still share things by using other common modules as well - if needed.

You can grant access to specific functions by marking them as pub(super) in the child module. This allows only the parent to access the type/function, so you can control visibility.

Example: https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-marshal-derive/src/marshalable.rs

Functional Changes

tpm2-rs-base

Remove #[repr(C)] from all of the types.

I think the purpose of this is to allow/enable zerocopy, but frankly zerocopy isn't actually being used from what I can tell. As such, defining this everywhere is currently not accomplishing anything. We can talk about zerocopy support in the future, but I think that currently it's practically not doing anything for this code. So there's no point in having attributes which don't accomplish anything.

Example: You can see this all over the sample repo, I removed these most everywhere.

tpm2-rs-errors

So, TpmError and TssError need some refactoring. I have an initial pass solution on this. Frankly I'm not sold on my rewrite, but I think it's a bit nicer than what we have today at least. I also think we can avoid macro_rules (though untested, so I may have messed up some of the logic, but it is just supposed to show the direction of the code), and if/when we can we should.

I also think that TssTspError is the right error type for marshaling, not TpmError (TPM error can only be reported from the TPM directly - not from us unmarshaling/marshaling something).

Example (TPM Error): https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-errors/src/tpm/tpm_error.rs
Example (TSS Error): https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-errors/src/tss/tss_error.rs

The TPM error constants thus can look like this: https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-errors/src/tpm/constants.rs

I think in the future it's easy to add support for mixing-in the associations with only the supporting error formats then. (Not shown in my example code, but you could imagine a function on TpmFmt1Error called with_association(association: TpmAssociation)). This separation of the types also allows us to limit things, so that format0 errors can't mix in an association (there is not even a function for it).

tpm2-rs-client

Frankly, I'm still thinking about how I would structure this.

I have some suggestions in the example repo, but I'm not sold on it. I think I'll make more concrete suggestions that take other things (like handles and sessions) into account later. I have some ideas, just haven't had time to explore them.

tpm2-rs-client-feature / tpm2-rs-service

I think we should avoid making any changes in these crates until we have the other crates nailed-down. If any of our changes break these crates, I'd just delete the code.

tpm2-rs-marshal(-derive)

I think we need to have a more structured type for writing, other than passing around a simple mutable buffer. That way the error handling can be contained in the type (kind of like the UnmarshalBuf but for marshaling), and we don't have to do this written += ...; stuff everywhere. Instead we should just have sequential readers and writers (that might be a good name for them too, since that's what they do).

Basically, let's not force the code to pass around byte offsets and raw mutable buffers - abstract that into a helper type. Then we can be more sure error handling is happening as expected (frankly it's a little unclear to me if the current marshalable can panic or not if an ill-sized buffer is passed somewhere).

I also think we can improve this to account for cases that we currently hardcode instead of using the derive macro. I suggest we add an attribute #[marshal(discriminant=<field>)] that will use a separately-named field for the discriminant of an enum (this will allow us to get rid of the manual definitions in some of the types).

Still thinking about the other cases, but I'm fairly convinced of the above. We should try to avoid implementing this manually when there's a path for implementing things in the proc-macro.

One other thing - there is a dependency on some internal functions defined by the proc-macro from outside of the proc-macro - this is not great. For now we can fix some of these by making them additional specific trait implementations:

pub trait MarshalableEnum: Marshalable {
  type Selector: Sized;
  fn discriminant(&self) -> Self::Selector;
  fn try_unmarshal_variant(selector: Self::Selector, buffer: &mut UnmarshalBuf) -> TssTspResult<Self>;
  fn try_marshal_variant(&self, buffer: &mut [u8]) -> TssTspResult<usize>;
}

But I think we can also update the marshalable to hide things in generated private submodules to avoid this happening again in the future. (This was found when moving code to separate modules.)

tpm2-rs-unionify(-derive)

I think that the buffer size calculations are closely related to Marshalable, so we should make those a part of the Marshalable trait instead of having a separate Unionify trait. (This will allow us to entirely remove the unionify and unionify-derive crates.)

---

If everyone is okay with all this, I'd be happy to start sending some small PRs to clean things up! If not, let's discuss it here and come to a consensus on a direction. I think the current code layout is very difficult to maintain, so I want to deal with that before we do anything else.

[bradlitterell]

I see that we're already adding some commands to the repo, and that's great - BUT I don't feel like the foundational elements of the code are very well-defined yet. I'd like for our focus to stay away from adding new commands for the time being, and towards cleaning up what we have (and adding tests, I think there's a lot of testing lacking).

+100 I love this!

[oddcoder]

Overall I agree, but I have few comments, but I must emphasize that those are just preferences based on similar pattern I see in other projects.

Avoid Nested Crates

That is generally good idea, but I prefer to have one exceptions to this rule. A Crate that implements #[derive(X)] should be sub directory in the the crate that contains the X trait, and it should be re-exported from the same level as Trait X. and ever accessed from the x-derive crate. I think we have couple of those instances around.

(Most) Types Should Exist In Their Own Modules

I feel this might be a bit too excessive for lots of the cases. For instance, this directory. It only contains type definitions, and no logic. Everything thing in it is exported. Many of the types in here are few lines of code at most.

I think a middle ground might be nicer, if a file describes data structures only with no (or every little) logic, we might want to bundle them together, so we would have tpmu.rs, tpmt.rs ....etc. It also feels in line with that rust itself does as in syn crate What do you think?

I think we should avoid making any changes in these crates until we have the other crates nailed-down. If any of our changes break these crates, I'd just delete the code.

I didn't contribute any of the code there, but I prefer to keep fixing things there as we go and avoid having to rewrite it again from scratch.

Instead we should just have sequential readers and writers (that might be a good name for them too, since that's what they do).

I have no preference regarding using arrays vs readers and writers, But if you prefer readers/writers, did you considered bytes. It should be heavily tested and well optimized.

[trent-reed]

That is generally good idea, but I prefer to have one exceptions to this rule. A Crate that implements #[derive(X)] should be sub directory in the the crate that contains the X trait, and it should be re-exported from the same level as Trait X. and ever accessed from the x-derive crate. I think we have couple of those instances around.

So, to be clear, Rust doesn't have such a thing as a sub-crate or private crate. All crate dependencies in a public crate must be published somewhere, and all published crates are in a flat list in crates.io. How we decide to lay our crates out in the repo has no bearing on how the crates are published to crates.io.

I agree with the re-exporting of the proc-macro, I don't want that to change. But I think that it's confusing to have a hierarchical structure of crates when there is currently no concept of that in rust.

I'd rather just have a flat list of crates that we publish, then it's really easy to see where everything is. If rust changes in the future to allow something like this, we can restructure at that time.

An example that does what I'm suggesting for derive is serde:

• https://github.com/serde-rs/serde/blob/master

You can see the main crate is serde, and that the derive crate is in a sibling directory named serde_derive.

They also re-export from serde_derive in serde (we can still do that, as said I'm fine with it): https://github.com/serde-rs/serde/blob/18dcae0a77632fb4767a420c550cb41991f750b8/serde/src/lib.rs#L324-L335

I feel this might be a bit too excessive for lots of the cases. For instance, this directory. It only contains type definitions, and no logic. Everything thing in it is exported. Many of the types in here are few lines of code at most.

As stated, it's not great in all cases, but I want it to be our default stance in most cases.

I agree this is getting to the point of personal-preference, but for me I like this separation even at the place you mentioned though. Here's a few reasons I could think of why I would separate into different files even at the location you mentioned:

1. These are public types defined from the spec, and we may wish to provide more detailed documentation on them. If we have them in separate files we can add as detailed of documentation as we want without worrying that it muddies up the file.
2. These types are not actually related to one-another, so structurally it seems wise to separate them to avoid cases where one derive accidentally depends on the private functionality of another (here is a great example of that happening in our current code: https://github.com/tpm-rs/tpm-rs/blob/e5fc33d7ae7fd3d8270f259d09b1f2be796f6115/base/src/lib.rs#L752; try_marshal_variant is a private function of TpmuAttest, but because it's in the same module as TpmsAttest it can access it - we can prevent accidental private dependencies like this in the future by separating into modules).
3. Frankly, I think it's easier to find a type by navigating to it instead of navigating to a file containing a bunch of types and then Ctrl+F to find the type. Plus the file explorer keeps the types sorted for us from a visibility perspective (yes, we'd have to manually sort the mod/use statements, but that's a bit easier to manage, and nobody is really expected to look at those).
4. Some types provide common helpers (like tpm2b) and if we have the child modules for the type implementations, this creates a nice structure for the macro to exist on its own separately from the implementations that use the macro: https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-base/src/types/tpm2b.rs
5. Some types have large definitions that would be wise to separate, and it seems weird to only separate some of the types but not all of the types for this spec-types (example: https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-base/src/types/tpm/tpm_cc.rs).

I'd admit to saying that I think tpm2b and friends are right on the line of when you should or shouldn't add a separate file for them. But my bid is that we do separate them in this case, because of the aformentioned reasons.

Here's an example of a place where I decided not to use separate files FWIW: https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-errors/src/tpm/tpm_error.rs#L35-L63

The idea being that these types don't have any non-derived impls, and they are closely related to TpmError and submodules, and truly they are very simple definitions. If the comments on these types become excessive, or the size of the enums or impls for them become large, I would later separate them into another file.

I didn't contribute any of the code there, but I prefer to keep fixing things there as we go and avoid having to rewrite it again from scratch.

My reason for saying this is I think work on those crates started before we were ready for it, so I suspect most of it will need re-written anyways. However, your point is valid - we can maintain what's there. Let's try not add more dependencies to the base crate until we have things stronger foundation though (unless the work done in those crates is very separate from the base types).

I have no preference regarding using arrays vs readers and writers, But if you prefer readers/writers, did you considered bytes. It should be heavily tested and well optimized.

I will take a look at the crate and see, thanks for the suggestion!

My main thing is that it just seems like work that we could define a simple helper type for instead of making it something that we have to codegen everywhere. I'll take a look at that crate later after some of the initial refactoring passes, and we'll make a call then.

[trent-reed]

I would like to clarify that my comment should be read as a discussion - if you feel strongly against anything that I say, please respond! I'm willing to bend on some of these things, I just want to explain why I feel the way I do about the structure I've proposed and what I thought about before suggesting it.

[oddcoder]

the only thing I have really strong opinion against is completely deleting old code written by others without providing alternative in the patch set that deleted it. It will always be possible to refactor from scratch, and there will always be the option to delete whole parts to make the refactor work. and we need to draw line around when it is no longer accepted to just restart from scratch, and my personal opinion is we should never do that.

[bradlitterell]

I think a middle ground might be nicer, if a file describes data structures only with no (or every little) logic, we might want to bundle them together, so we would have tpmu.rs, tpmt.rs ....etc. It also feels in line with that rust itself does as in syn crate What do you think?

Most of the TPM types derive between TPMU/TPMT/TPM2B in order, so keeping those in the same module probably makes sense to avoid extra flipping back and forth when reading/writing.

e.g. consider TPM2B_DIGEST (in the 'C' version of TpmTypes.h):

typedef union {
/* snip - various sized hash buffers... */
} TPMU_HA;

typedef struct
{  // (Part 2: Structures)
    TPMI_ALG_HASH hashAlg;
    TPMU_HA       digest;
} TPMT_HA;

typedef union
{  // (Part 2: Structures)
    struct
    {
        UINT16 size;
        BYTE   buffer[sizeof(TPMU_HA)];
    } t;
    TPM2B b;
} TPM2B_DIGEST;

This suggests to me a tpmt-digest.rs meaning where tpmt means "tpm type" and includes the digest related types. (or simply tpm-digest.rs if you prefer, but I see value in separating types from implementation and in at least some cases (like ECC/RSA) I expect naming issues if the file prefix is tpm instead of tpmt.

[bradlitterell]

Or in Rust, maybe we only have structures containing the union tag, more like TPMT with an embedded TPMU instead of having a separate TPMU type? I'm not sure how much value attains to having separate union types since they aren't very practical without the type tag.

[trent-reed]

Most of the TPM types derive between TPMU/TPMT/TPM2B in order, so keeping those in the same module probably makes sense to avoid extra flipping back and forth when reading/writing.

This is a good point, those types are related to one-another so I can see the desire to keep them in the same module, close to one-another for readability-sake. However, I'd bet that we still want the separation at a prefix-level (tpmu, tpmt, tpm2b, etc.) for a few reasons:

1. If we have common code that matters for all types under a certain group, then it gives us a sensible parent module to implement the common code in (examples: https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-base/src/types/tpm2b.rs, https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-base/src/types/tpma.rs, https://github.com/trent-reed/tpm-rs/blob/dev/trent-reed/restructuring/tpm2-rs-base/src/types/tpml.rs)
2. If we want to add unit tests that are generic over a kind of type, it will make sense to add those to tpmu, tpmt, etc.

I think having the organization structure being the prefix makes enough sense, it may not be perfect but it's good enough.

And for clients, they will probably refer to documentation anyways, and for documentation they'll all appear in the same path (because of the re-exports), so really we just need to do what would make sense ergonomically for developing the code.

[trent-reed]

Hey All!

I opened a poll for us to decide on the direction here (since most of the discussion has been about how to organize the submodules). Please respond there!

Poll: #19

Aside from this I believe I have responded to the other concerns.

1. I feel unconvinced in having nested crates of any kind, including with derive crates. Unless I get strong push-back I'm going to move forward with making the crates a flat list in the root.
2. We will try to maintain code in service and client-feature, instead of deleting it. (A good call-out honestly, I think I was too eagerly trying to get rid of code. I suspect we will still massively refactor code in these crates though.)
3. We'll look into what types to use instead of raw buffers after cleaning up the derive code.

I will let this discussion stay open throughout the rest of the week! So if you want your voice heard, or disagree with anything I said please respond before then!

[oddcoder]

1. I feel unconvinced in having nested crates of any kind, including with derive crates. Unless I get strong push-back I'm going to move forward with making the crates a flat list in the root.

That would be fine by me as long as symbols are re-exported, perhaps we should add guidelines (perhaps in the readme) to never import x-derive manually

2. We will try to maintain code in service and client-feature, instead of deleting it. (A good call-out honestly, I think I was too eagerly trying to get rid of code. I suspect we will still massively refactor code in these crates though.)

Good idea

3. We'll look into what types to use instead of raw buffers after cleaning up the derive code.

After this is decided, I can help with refactoring if you wouldn't mind.It will be good if we can co-ordinate on that to avoid duplicate work and to speed up the time frame by which we have an base that everyone agrees on.

[trent-reed]

Alright, I'm guessing we're at the end of this discussion at this point. Note that the poll (#19) for how we will split up the specification base types will still be open until the middle of the week (Wednesday ~3PM PST) - if you care to please let your voice be heard!

I will note that this will not be the only discussion about codestyle, layout, norms, etc that we will have. There's more to discuss, I just wanted to limit it to a few topics for the first discussion to break it up a bit.

Next Steps:

1. I'll start by sending some PRs to do the initial restructuring. (This should be done by one person to avoid us stepping on each-other's toes I think).
2. This week I'll go over the outstanding issues list and respond to those that I have an opinion on, and I'll start opening issues so that it's clear what the next work items to be shared can be.
3. I'll try to find someplace where we can document these code norms that we decide upon, that way we don't accidentally redo these discussions in the future, or they don't get lost. (Probably this'll be some markdown file in one of the other repos somewhere.)
4. I hope that by next week we'll be back to a state where multiple people can work on the project without stepping on any toes.

After (3), I will close this discussion to keep the open discussion list clean. I'm trying to become a maintainer (PR) so that I can clean up what is already there, too.

Thanks to those of you who contributed to this discussion!