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

Improves handling of size_t FFI values. #471

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

Superhepper
Copy link
Collaborator

@Superhepper Superhepper commented Nov 8, 2023

@Superhepper Superhepper force-pushed the size_t-handling branch 4 times, most recently from 65e1a61 to 4a6e638 Compare November 9, 2023 20:15
@Superhepper Superhepper changed the title Improves handling size_t FFI values. Improves handling of size_t FFI values. Nov 9, 2023
Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch, looks good so far!

tss-esapi/src/ffi.rs Outdated Show resolved Hide resolved
tss-esapi/src/interface_types/structure_tags.rs Outdated Show resolved Hide resolved
@Superhepper Superhepper force-pushed the size_t-handling branch 3 times, most recently from 316fc8e to c163fac Compare November 15, 2023 19:33
@Superhepper Superhepper marked this pull request as ready for review November 15, 2023 19:34
wiktor-k
wiktor-k previously approved these changes Nov 16, 2023
Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general it looks good and a lot of code got cleaner. I'm not a fan if leaving the commented-out code though but besides that 👍 Thanks!

tss-esapi/src/structures/buffers/private.rs Outdated Show resolved Hide resolved
@Superhepper
Copy link
Collaborator Author

@wiktor-k
Haha sorry I will remove that. I needed it as a reference when I write the macros.

Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the effort! I hate how similar, yet just different enough the implementations of Marshall and UnMarshall have to be...

let mut buffer = vec![0; Self::BUFFER_SIZE];
let mut offset = 0;

fn marshall_offset(&self, marshalled_data: &mut [u8], offset: &mut usize) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one doesn't fit the macro patterns?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

let mut dest = 0_u32;
/// Macro used to implement Marshall and Unmarshall for types that
/// can be converted from native to TSS i.e. it cannot fail and are
/// passed to MUAPI by value(i.e. pointer).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by value (i.e. pointer) - is this correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

}

// Make the macros usable outside of the module.
pub(crate) use impl_marshall_trait;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually imported somewhere else?

Copy link
Collaborator Author

@Superhepper Superhepper Nov 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but I thought it could be useful if you for some reason only wanted to implement one of the traits or there was a case where the implementation did not fit the other macros.

And I had some problem getting it to build if I did not export them as well.

- The size_t type that is used in all the MU APIs will
  be dependent on the compiler that ```bindgen``` uses to
  generate the bindings for the library. This attempts to
  make the crate less dependant on that value in the
  public APIs.

- Changes the Marshall and Unmarshall traits to use usize as
  input.

- Removed the ```private_in_public``` it is 'warn by default' and
  because documentation ci is run with ```-Dwarnings``` it will
  cause the lint to be enforced.

Signed-off-by: Jesper Brynolf <[email protected]>
@Superhepper Superhepper merged commit 2dfc315 into parallaxsecond:main Nov 30, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect handling of FFI size_t in marshalling and unmarshalling trait implementation.
4 participants