Skip to content

Make X509Store shareable between contexts#366

Merged
kornelski merged 1 commit intomasterfrom
store-by-ref
Jun 5, 2025
Merged

Make X509Store shareable between contexts#366
kornelski merged 1 commit intomasterfrom
store-by-ref

Conversation

@kornelski
Copy link
Collaborator

@kornelski kornelski commented Jun 4, 2025

nox
nox requested changes Jun 5, 2025
View reviewed changes
Copy link
Collaborator

@nox nox left a comment

Choose a reason for hiding this comment

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

I don't like that you introduced a panic in cert_store_mut, as it can make currently existing code panic now when combined with set_cert_store. Let's wait for boring 5.

@kornelski
Copy link
Collaborator Author

kornelski commented Jun 5, 2025

Existing code won't panic, because I haven't changed the semantics of set_cert_store.

It think it's better to deprecate set_cert_store first in v4, and then make a breaking behavior change in v5. The current API will force users to decide if they want immutable or mutable store by switching to one of the new methods.

@nox
Copy link
Collaborator

nox commented Jun 5, 2025 •
edited
Loading

Existing code won't panic, because I haven't changed the semantics of set_cert_store.

Existing code can do .set_cert_store(store) and then .cert_store_mut().

Never mind.

@nox
Copy link
Collaborator

nox commented Jun 5, 2025

I assumed that the PR also implemented Clone for X509Store hence my confusion, I'm ok with the changes now.

@nox
Copy link
Collaborator

nox commented Jun 5, 2025

This is clever btw, nice of you to find a way without having to do boring 5.

@0x676e67
Copy link
Contributor

0x676e67 commented Jun 5, 2025

It looks good, so it can also be referenced in the external Arc packaging Cert Store.

@kornelski kornelski merged commit 5d57b3a into master Jun 5, 2025
25 checks passed
@kornelski kornelski deleted the store-by-ref branch June 5, 2025 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nox nox nox approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kornelski @nox @0x676e67