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

bindings: Create 32b compatible bindings #107

Merged
merged 1 commit into from
Oct 29, 2024
Merged

bindings: Create 32b compatible bindings #107

merged 1 commit into from
Oct 29, 2024

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Oct 29, 2024

It turns out the bindgen assumes 64b architecture by default when converting constants, which causes warnings like

warning: literal out of range for `u32`
 --> src/pkcs11/bindings.rs:7:50
  |
7 | pub const CK_UNAVAILABLE_INFORMATION: CK_ULONG = 18446744073709551615;
  |                                                  ^^^^^^^^^^^^^^^^^^^^
  |
  = note: the literal `18446744073709551615` does not fit into the type `u32` whose range is `0..=4294967295`
  = note: `#[warn(overflowing_literals)]` on by default

but more important, most of the tests keep failing as the comparisons around this value wont work.

Rather than inventing a way to convert the constant to c-type ulong, I thought setting specific value known as to be the MAX would be more obvious.

Builds run in rawhide now:

https://copr.fedorainfracloud.org/coprs/jjelen/kryoptic/build/8186035/

Unfortunately, this did not fix it and we are still getting error during intialization with:

---- tests::token::test_token_sql stdout ----
thread 'tests::token::test_token_sql' panicked at src/tests/mod.rs:97:43:
called `Result::unwrap()` on an `Err` value: Error { kind: CkError, origin: None, errmsg: None, reqsize: 0, ckrv: 19 }

(CKR_ATTRIBUTE_VALUE_INVALID), which sounds like some attributes pulled from DB have different bit size than expected by the 32b build. Not sure if there is something in #106, which might fix it -- I will try after it will get merged.

The Fedora 41 builds seems to be failing for unrelated reasons. Sounds like the bindings were not generated correctly and the types with blocklisted prefixes are not generated as they should on the Fedora 41 version of bindgen? -- still investigating.

src/pkcs11/interface.rs Outdated Show resolved Hide resolved
@simo5
Copy link
Member

simo5 commented Oct 29, 2024

@Jakuje given you are updating bindgen related stuff, would you mind updating to bindgen 0.70 (desirable as it drops a bunch of dependencies), and see if the problem persists with 0.70?

If it does your solution is ok with me for now.

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

See comment above

@Jakuje
Copy link
Contributor Author

Jakuje commented Oct 29, 2024

@Jakuje given you are updating bindgen related stuff, would you mind updating to bindgen 0.70 (desirable as it drops a bunch of dependencies), and see if the problem persists with 0.70?

This might be an issue for the Fedora packaging, as the Fedora package is still on 0.69, see https://src.fedoraproject.org/rpms/rust-bindgen.

Anyway, I opened rust-lang/rust-bindgen#2965 as it reproduces with both 0.69 and 0.70. I would prefer to keep it as proposed for now. Maybe I will just add a comment to the commit/code to make sure to remove the workaround as we will have fix.

This is a workaround for a bindgen, not setting the correct value
for unsigned long constants:

rust-lang/rust-bindgen#2965

Remove when the above issue is fixed.

Signed-off-by: Jakub Jelen <[email protected]>
@simo5 simo5 merged commit 9e11dca into latchset:main Oct 29, 2024
5 checks passed
@simo5
Copy link
Member

simo5 commented Oct 29, 2024

Thanks, this works for now!

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.

2 participants