Make Name wrap the raw type directly

[wiktor-k]

Hi,

I've noticed that in the cryptoki crate most wrapper types just directly contain the native/raw type. This makes it very efficient to pass around.

Usually it's the low level function that gives and accepts the raw types and using the wrapper design, without intermediate Vec<u8> buffer makes it possible to borrow immutable refs to the inner type (via AsRef<T>). This allows reusing one reference thus reducing the number of clones needed.

(Edit: I think some Rust types also use this approach of exposing inner types, e.g. Vec<u8> implements AsRef<&[u8]> and str implements AsRef<[u8]>).

In this example PR I just did that to Name but I think more types could benefit from this approach too but since I'm not a super-experienced Rust programmer I'll leave the PR here for discussion on whether this approach is worth to explore.

See you later! 👋

CC: @ionut-arm, @Superhepper, @hug-dev.

[ionut-arm]

I like the idea: as long as we don't sacrifice safety and the API remains fairly Rust-native, it would be pretty useful! In terms of safety it's stuff like self.value.name[..self.value.size as usize] that I'm afraid of, but as long as the structures are immutable and we have good checks on constructors it should be fine. Don't know if we can apply this to all types, but it would probs work for buffers, for example.

[ionut-arm]

I think you should have a method on Name that just takes ownership of the value and returns a TPM2B_NAME, that way you don't have to clone here 😄 Or have you tried and it doesn't work?

[wiktor-k]

There is impl TryFrom<Name> for TPM2B_NAME did you mean that? I just removed it for the sake of the argument but maybe we could have impl From<Name> for TPM2B_NAME instead of TryFrom. The reasoning is that if we validate TPM2B_NAME when constructing Name then when converting it back the inner TPM2B_NAME is correct by design.

[ionut-arm]

Sure, yeah, something like that works!

[wiktor-k]

I think this also ties nicely to this issue: #223 :)

Thanks for the input @ionut-arm, I greatly appreciate it!

If you don't mind I'll wait and listen to what @Superhepper and @hug-dev have to say :) (because if this is a good idea I could file a couple more PRs improving internal representation of types).

[ionut-arm]

Did you have a list of types you were targeting with this refactoring? Might be worth creating an issue and putting them in a list there - if they're all kinda the same it might not lead to much discussion, but maybe for some of them we can find other ways of abstracting

[wiktor-k]

🤔 I've got a list of potential candidates:

$ git grep 'impl TryFrom.*for TPM'
src/attributes/nv_index.rs:impl TryFrom<NvIndexAttributes> for TPMA_NV {
src/nv/storage/public.rs:impl TryFrom<NvPublic> for TPM2B_NV_PUBLIC {
src/structures/hash/agile.rs:impl TryFrom<HashAgile> for TPMT_HA {
src/structures/lists/digest.rs:impl TryFrom<DigestList> for TPML_DIGEST {
src/structures/lists/digest_values.rs:impl TryFrom<DigestValues> for TPML_DIGEST_VALUES {
src/structures/tagged/signature.rs:impl TryFrom<Signature> for TPMT_SIGNATURE {
src/structures/tagged/symmetric.rs:impl TryFrom<SymmetricDefinition> for TPMT_SYM_DEF {
src/utils/mod.rs:impl TryFrom<TpmsContext> for TPMS_CONTEXT {

I've made Name impl From instead of TryFrom in this PR and that allowed CreationData to also use From instead of TryFrom and this is probably similar with types that I've listed: making Digest impl From would probably affect other values (e.g. HashAgile).

Another thread to think about: quite a lot of these fallible conversions appear in unsafe blocks when calling Esys functions. Is checking what the underlying TPM returns desirable? Because if it returns bad data then the user already has bigger problems to bother with (e.g. the TPM is non-compliant) and I'm not sure if this crate should also work as a "compliance checker". What if we add unsafe fn from_raw_unchecked(val: TPM_RAW_TYPE) and use that in unsafe blocks... 🤷

[Superhepper]

In general I think it would be a good thing if we could reduce the number of clones needed. If using the TPM type as the underlaying type for the wrappers is a good solution that I am not so sure about. Because some of the types should really not be used like the TPMT types because that one has a union and a selector and the rust variant is much safer. And some of them are perhaps not as flexible as we would like them to be like the TPML types. The TPMS types consist of several different TPM types which could perhaps make them a little bit troublesome to use as the underlaying type. The TPM2B types are sized buffers and perhaps most suited to have the TPM type as an underlaying type as long as we can ensure that the use of self.value.name[..self.value.size as usize] will not result safety concerns.

[wiktor-k]

Good points @Superhepper! So, for buffers we can embed the type and avoid clones that way. For others this won't work. Still, this is something.

Can I assume you're both OK with this PR then?

[Superhepper]

Yeah, I would not be against it as long as we check the data properly and that brings me to the other point.

About the checking what the TPM returns, I actually like that. It makes it possible to exit early when something is not right. Instead of fooling a user that everything is good when it really isn't. I feel like that when the data enters the code of the crate we should make an effort to make sure it is correct.

[wiktor-k]

Understood. Thanks for taking the time to describe your point of view!