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

Add streaming API #2

Merged
merged 5 commits into from
Apr 26, 2023
Merged

Add streaming API #2

merged 5 commits into from
Apr 26, 2023

Conversation

sosthene-nitrokey
Copy link
Collaborator

No description provided.

@sosthene-nitrokey
Copy link
Collaborator Author

This makes me realise that the approach of having multiple extensions implemented in one backend is a bit complex on the side of the dispatcher, because there are multiple implementation of the same methods where just the trait changes, so the compiler can't infer which actual implementation to use. This leads to code like

<StagingBackend as ExtensionImpl<ChunkedExtension>>::extension_request_serialized(
    &mut self.backend,
    &mut ctx.core,
    &mut ctx.backends,
    request,
    resources,
)

It's also a semver hazard as self.backend.extension_request_serialized(&mut ctx.core, ctx.backends,request,resources) compile fines if only one extension is enabled through feature-flags, but it doesn't if there are more.

@sosthene-nitrokey
Copy link
Collaborator Author

Maybe we should add a dummy extension that is always enabled. That way it forces everyone to use the fully qualified path instead of relying on type inference when only one extension is enabled.

@robin-nitrokey
Copy link
Member

Also, we will probably have the same issue as with trussed-auth where a breaking change in the backend requires us to bump the dependency version for the clients that only use the extension. What do you think about having the extensions as separate crates within this repository, e. g. trussed-staging-streaming, and then have one backend, trussed-staging, that implements them? This would combine the benefits of a single backend (less overhead) with the benefits of separate extension crates (fewer breaking changes).

@sosthene-nitrokey
Copy link
Collaborator Author

I see the point regarding breaking changes and semver,but I don't see how this would fix the naming when implementing dispatch

Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

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

Looks good! Maybe I missed it but helper functions that take a &[u8] or return a Bytes<N> and implement the chunking would also be useful.

@sosthene-nitrokey
Copy link
Collaborator Author

There is the write_all helper function in utils that takes a &[u8] of any length and performs the chunking. I didn't want to do a method that takes or return a Bytes<N> because the whole point of the streaming API is for file that are too large and for which we don't want to have large stack allocations.

What I think we may want to do some function that reads a file and insert it to a &mut Vec, but I was not sure which type should be used?

In opcard and PIV we have a Reply wrapper type around Bytes which are themselves a wrapper around heapless::Vec<u8,N>.

Maybe we could have something like read_to_end(path, location, buffer: &mut impl Write) but I'm not sure which Write trait we should use (the std::io::Write is not available in core). Maybe we should come up with our own?

@sosthene-nitrokey
Copy link
Collaborator Author

Also there are many cases where we want to have the length of the file before we begin writing stuff to the output. This is for example the case when dealing with TLV data.

@robin-nitrokey
Copy link
Member

I think an ExtendFromSlice trait would be useful. I’ve had a similar problem when trying to make iso7816 more generic. Anyway, this should not block this PR. The write helper is probably much more useful than a read helper anyway.

@sosthene-nitrokey
Copy link
Collaborator Author

I created #3 and #4 for the issues raised here. I think we can merge it this way for now.

@robin-nitrokey robin-nitrokey merged commit 7cdb99b into main Apr 26, 2023
@robin-nitrokey robin-nitrokey deleted the streaming branch April 26, 2023 13:05
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