Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace TryFrom with From where it makes sense #223

Open
ionut-arm opened this issue Apr 16, 2021 · 1 comment
Open

Replace TryFrom with From where it makes sense #223

ionut-arm opened this issue Apr 16, 2021 · 1 comment
Labels
medium Effort label

Comments

@ionut-arm
Copy link
Member

Most of our native types in the tss-esapi crate implement conversions to and from the FFI types coming from tss-esapi-sys. The types follow some strict rules:

  • when converting from FFI type to native, we perform checks to make sure all parameters are valid, returning an error if not (implemented as TryFrom)
  • we do not allow direct access to the internal data of these values, using getters instead
  • we do not allow constructors that take in random data and instead prefer either constructors that enforce the invariants that the type is subject to (e.g. max size of a buffer) or conversions that do the same

I think for any type for which we can be fairly confident about (in terms of "any value of this type is a valid representation of the underlying FFI type") we should use From conversions when going from native to FFI. This is mostly to avoid the extra trouble of dealing with potential errors when we're certain they cannot happen. This is particularly important for "low-level" types (such as Name) which get incorporated into many other types, forcing the use of TryFrom for those as well.

There could be cases/types for which we can reasonably expect more heavy work in the future (more conversions or more methods on them?), and maybe for those it doesn't make sense to make this change, just in case we mess something up and the type isn't as safe as we think.

@ionut-arm ionut-arm added the medium Effort label label Apr 16, 2021
@Superhepper
Copy link
Collaborator

I agree when the native types are checked upon creation there is no point in checking them upon conversion unless you expect some cosmic radiation has caused single bit upset some where could result in the data being corrupted between the time of creation and the time of conversion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium Effort label
Projects
None yet
Development

No branches or pull requests

2 participants