Module tree hygiene #56

vkkoskie · 2021-10-31T04:26:14Z

This PR introduces breaking changes. It attempts to resolve Issue #55, which has a more complete description of its rationale.

The changes are almost entirely structural. No function bodies or prototypes have changed, and the overall API is identical. The break comes only in the paths through which external users of the crate access its items. (e.g., use cryptoki::types::session::Session is now use cryptoki::session::Session but the Session struct is unchanged). The overall effect is a wider, shallower tree, where each module contains conceptually linked types. This produces a much more intuitive "front door" for documentation.

Because file move diffs are often difficult to read, the PR is broken up into a series of commits each with its own detailed commit message to make review easier.

As I was making these changes, I also noticed a general misuse of visibility markers, and I suspect that more of the crate is exposed as pub than is ideal. While I did make several necessary changes from private to pub(crate) in order for moved items to be usable with the new structure, I only made one visibility change unrelated to this PR's primary goal. This is the final commit to the branch and removes the macro_export directive to take the get_pcks11 macro out of the public API (where it wouldn't have compiled anyway).

closes #55

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

The 'functions' module subtree contains additional impl blocks for the Session and Pkcs11 structs. The pub elements of those blocks are accessible through those types whether or not their containing module is pub, so these just create extra documentation clutter. Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Make the `types` module `pub(crate)` and re-export its contents through a new module structure. The new structure is intended to better group related concepts to make documentation easier to navigate. As a side effect, it also hoists all definitions up a level, making the tree wider, with individual items easier to navigate to. While the backing types module remains substantively unchanged, this does break the paths through which items are accessed in client code (see test diffs for examples). Version bumped to 0.3.0 to reflect this. Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

This moves the mechanism module out of types and into the location indicated in commit 20b26a5. src/types/mechanism/* -> src/mechanism/* src/types/mod.rs::MechanismFlags -> src/mechanism/flags.rs Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

This moves the sesson module out of types and into the location indicated in commit 20b26a5. src/types/session.rs -> src/session/mod.rs src/types/mod.rs::SessionFlags -> src/session/flags.rs Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

This moves the object module out of types and into the location indicated in commit 20b26a5. src/types/object.rs -> src/object.rs Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

This moves the slot module out of types and into the location indicated in commit 20b26a5. src/types/slot_token.rs -> src/slot/mod.rs src/types/mod.rs::{SlotFlags,TokenFlags} -> src/slot/flags.rs Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

This creates the context module out of the init module initially defined in commit 20b26a5 as well as main ctx definition from lib.rs. src/lib.rs::Pkcs11 -> src/context/mod.rs src/lib.rs::get_pkcs11 -> src/context/mod.rs src/types/locking.rs -> src/context/locking.rs src/types/mod.rs::Info -> src/context/info.rs src/types/mod.rs::InitializeFlags -> src/context/flags.rs Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

This moves the Rv enum out of the types module to match the crate scope it was placed at in commit 20b26a5. This allows the remainder of the contents of types to be restored by removing the prior misc definition. src/types/mod.rs -> src/types.rs src/types/function.rs -> src/rv.rs Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Move previously crate-scope Error, Result, Rv, and RvError into their own module Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

The get_pcks11() accessor macro assumes it has access to private members of the Pkcs11 struct. As a result, any use of it outside this crate will fail to compile. This commit removes `macro_export` attribute which exposes it to client code. Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

mike-boquard · 2021-11-02T00:56:11Z

Any reason not to just delete the objects module all together as it's empty and I, at the moment, cannot think of a good reason to keep it?

ionut-arm

The changes look sensible to me! Also, I'm happy with the objects module being removed.

The imports for that failing doctest should also be fixed, though

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

The functions module contains impl blocks for both the Session and Pcks11 structs. This commit moves those blocks into the respective modules where those structs are defined. Files session_management.rs and slot_token_mangement.rs contained impl blocks for both, and were split in the process. Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

This commit is much easier to view with whitespace ignored. Each method of the Session and Pcks11 structs implemented in a submodule is now exposed as a stub function. This has several advantages: * Functions remain internally categorized as in the PKCS#11 specification, while exposing a smooth interface. Previously each impl block, despite belonging to a single struct, would be shown in the public-facing docs, which created a lot of visual clutter. * The stubbed methods in the public modules can now collect arbitrarily large documentation blocks, without making the code itself difficult to navigate by being in one monolithic file Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

vkkoskie · 2021-11-04T01:45:02Z

New commits. More file moves, which make diffs look much more drastic than they really are. Here's the summary:

To try and respect the prior organization as much as possible, the methods previously located in the functions module all still occupy a file of the same name, indicating which section of the specification they belong to. However, all have been changed from methods to private functions in a submodule of the struct they belong to. The struct definition itself exposes these functions through method stubs, which are now also the collection point for method documentation.

This provides what I think is a nice balance between the needs of developers and users of the crate. Users see a uniform struct uncluttered by impl annotations. This makes the struct itself monolithic which would be cumbersome to developers, but instead they get to work in the broken out files nearby.

I wasn't sure if performance was of any concern, but I went ahead and removed the layer of indirection introduced by stubbing by forcing explicit inlining. Even if performance isn't a big concern, it does no harm.

I also found what I think is a bug in Session::close(), which I believe was intended to take ownership of self to trigger a call to drop() the same way the library context does. It probably should have been its own Issue/PR, but I didn't think it would be a big deal to include it.

mike-boquard · 2021-11-04T02:15:44Z

cryptoki/src/session/mod.rs

@@ -88,7 +88,7 @@ impl Session<'_> {

    /// Close a session
    /// This will be called on drop as well.
-    pub fn close(&self) {}
+    pub fn close(self) {}


That's a good catch, otherwise close() really didn't do anything.

mike-boquard · 2021-11-04T02:28:26Z

I think I buy why you made those changes in b711d0c. It's definitely drastic but the docs that are generated are significantly improved.

cryptoki/src/session/mod.rs

vkkoskie · 2021-11-04T02:31:47Z

Hmm. Github CI is unhappy with a part of the code I didn't touch. Maybe the rust image updated to a new version? Just a guess.

cryptoki/src/session/mod.rs

Co-authored-by: Mike <michael.j.boquard@gmail.com> Signed-off-by: Michael J Boquard <michael.j.boquard@gmail.com>

Fix rustdoc tests Signed-off-by: Michael J Boquard <michael.j.boquard@gmail.com>

mike-boquard · 2021-11-04T03:28:49Z

Went ahead and made the changes and force pushed an updated commit to contain the 'magic' 'Signed-off-by' line.

vkkoskie added 12 commits October 31, 2021 00:30

Don't expose empty module as pub

3ebeb13

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Move mechanism module (non-breaking)

06894f0

This moves the mechanism module out of types and into the location indicated in commit 20b26a5. src/types/mechanism/* -> src/mechanism/* src/types/mod.rs::MechanismFlags -> src/mechanism/flags.rs Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Move session module (non-breaking)

48c694b

This moves the sesson module out of types and into the location indicated in commit 20b26a5. src/types/session.rs -> src/session/mod.rs src/types/mod.rs::SessionFlags -> src/session/flags.rs Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Move object module (non-breaking)

3a0eafd

This moves the object module out of types and into the location indicated in commit 20b26a5. src/types/object.rs -> src/object.rs Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Move slot module (non-breaking)

f8afc46

This moves the slot module out of types and into the location indicated in commit 20b26a5. src/types/slot_token.rs -> src/slot/mod.rs src/types/mod.rs::{SlotFlags,TokenFlags} -> src/slot/flags.rs Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Create error module (breaking)

ef03ee3

Move previously crate-scope Error, Result, Rv, and RvError into their own module Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

cargo fmt

69b197f

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

vkkoskie force-pushed the module-tree-hygiene branch from 17bcefa to 454310c Compare October 31, 2021 04:37

vkkoskie changed the title ~~Module tree hygiene~~ Module tree hygiene (closes #55) Oct 31, 2021

vkkoskie changed the title ~~Module tree hygiene (closes #55)~~ Module tree hygiene Oct 31, 2021

ionut-arm self-requested a review November 2, 2021 09:20

mike-boquard self-requested a review November 2, 2021 12:43

ionut-arm approved these changes Nov 3, 2021

View reviewed changes

vkkoskie added 6 commits November 3, 2021 15:44

Remove empty objects module

b49efe4

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Remove unnecessary lifetimes from Session impls

3f1855e

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

BUGFIX: Session::close must take ownership

9430f80

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

Force inlining of all functions exposed as stubs

b7d4b86

Signed-off-by: Keith Koskie <vkkoskie@gmail.com>

mike-boquard reviewed Nov 4, 2021

View reviewed changes

mike-boquard requested changes Nov 4, 2021

View reviewed changes

cryptoki/src/session/mod.rs Outdated Show resolved Hide resolved

mike-boquard enabled auto-merge November 4, 2021 02:30

auto-merge was automatically disabled November 4, 2021 02:34
Head branch was pushed to by a user without write access

mike-boquard reviewed Nov 4, 2021

View reviewed changes

cryptoki/src/session/mod.rs Outdated Show resolved Hide resolved

vkkoskie and others added 2 commits November 3, 2021 23:27

Suppress run of session method doc test

2430ccb

Co-authored-by: Mike <michael.j.boquard@gmail.com> Signed-off-by: Michael J Boquard <michael.j.boquard@gmail.com>

Update cryptoki/src/session/mod.rs

7919c61

Fix rustdoc tests Signed-off-by: Michael J Boquard <michael.j.boquard@gmail.com>

mike-boquard force-pushed the module-tree-hygiene branch from e7dafef to 7919c61 Compare November 4, 2021 03:27

mike-boquard enabled auto-merge November 4, 2021 03:28

mike-boquard approved these changes Nov 4, 2021

View reviewed changes

mike-boquard merged commit abd872a into parallaxsecond:main Nov 4, 2021

vkkoskie deleted the module-tree-hygiene branch November 4, 2021 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module tree hygiene #56

Module tree hygiene #56

vkkoskie commented Oct 31, 2021 •

edited

Loading

mike-boquard commented Nov 2, 2021

ionut-arm left a comment

vkkoskie commented Nov 4, 2021

mike-boquard Nov 4, 2021

mike-boquard commented Nov 4, 2021

vkkoskie commented Nov 4, 2021

mike-boquard commented Nov 4, 2021

Module tree hygiene #56

Module tree hygiene #56

Conversation

vkkoskie commented Oct 31, 2021 • edited Loading

mike-boquard commented Nov 2, 2021

ionut-arm left a comment

Choose a reason for hiding this comment

vkkoskie commented Nov 4, 2021

mike-boquard Nov 4, 2021

Choose a reason for hiding this comment

mike-boquard commented Nov 4, 2021

vkkoskie commented Nov 4, 2021

mike-boquard commented Nov 4, 2021

vkkoskie commented Oct 31, 2021 •

edited

Loading