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

MAX_MESSAGE_SIZE configuration granularity is insufficient #330

Open
chrysn opened this issue Jan 16, 2025 · 2 comments · May be fixed by #331
Open

MAX_MESSAGE_SIZE configuration granularity is insufficient #330

chrysn opened this issue Jan 16, 2025 · 2 comments · May be fixed by #331

Comments

@chrysn
Copy link
Collaborator

chrysn commented Jan 16, 2025

There's a note in shared:

// TODO: find a way to configure the buffer size

The current workaround is to have a feature quadruple_sizes that scales everything up; turns out that for at least one application, 4x is so large it smashes the stack, and 1x is too small for ACE tokens.

I've played around with envinroment variables, and found something that works:

-// TODO: find a way to configure the buffer size
-// need 128 to handle EAD fields, and 192 for the EAD_1 voucher
-pub const MAX_MESSAGE_SIZE_LEN: usize = SCALE_FACTOR * (128 + 64);
+pub const MAX_MESSAGE_SIZE_LEN: usize = if let Some(e) = option_env!("LAKERS_MAX_MESSAGE_SIZE") {
+    match konst::primitive::parse_usize(e) {
+        Ok(n) => n,
+        Err(_) => panic!("Error parsing LAKERS_MAX_MESSAGE_SIZE"),
+    }
+} else {
+    // need 128 to handle EAD fields, and 192 for the EAD_1 voucher
+    SCALE_FACTOR * (128 + 64)
+};

But I'm not happy with that: It requires the environment variable to be set from outside, and that means that the crate that has the requirement somewhere down in the tree needs to parse the environment variable as well, and complain, and then the user has to call it with some variable … nah.

I do now think that the better form is to have granular lakers_max_message_size_256 etc. features, in arbitrary steps, which can be selected by downstream crates; that seems to be the common practice in other embedded crates as well.

A prettier solution is of course to introduce generics, but that'll need more changes, and I'd rather have something working soon.

chrysn added a commit to chrysn-pull-requests/edhoc-rs that referenced this issue Jan 16, 2025
@chrysn chrysn linked a pull request Jan 16, 2025 that will close this issue
chrysn added a commit to chrysn-pull-requests/edhoc-rs that referenced this issue Jan 17, 2025
@chrysn
Copy link
Collaborator Author

chrysn commented Jan 17, 2025

Two things I've added:

  • Features for other buffer sizes I ran into
  • I've moved the branch onto 0.7.2. It works with main too, so merging it is fine, but to avoid working from different APIs, I need it to be usable before the message 4 API change was merged.

@geonnave
Copy link
Collaborator

Thanks for raising this and referencing embassy-executor. This approach seems fine to me.

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 a pull request may close this issue.

2 participants