Replumbing: Add the Provider Object, type OSSL_PROVIDER by levitte · Pull Request #8287 · openssl/openssl

levitte · 2019-02-19T22:20:23Z

The OSSL_PROVIDER is the core object involved in loading a provider
module, initialize a provider and do the initial communication of
provider wide and core wide dispatch tables.

Checklist

documentation is added or updated
tests are added or updated

Note: this is made on top of #8286, ensure that you know which is which

levitte · 2019-02-19T22:20:54Z

TODO: docs and test. That will be for tomorrow

levitte · 2019-02-20T22:09:19Z

This now includes a few tests that contain a very simple provider that provides nothing but a greeting. This simple provider gets to act both as a built in provider and as a loadable provider module.

Count this as official ~~first~~very early boot!

levitte · 2019-02-20T22:09:58Z

TODO: documentation

levitte · 2019-02-21T05:25:12Z

Hmmm, Travis fails the loaded provider module test with a segfault. The tests work right on all machines I can get my hands on for the moment... so does anyone else have a clue?

levitte · 2019-02-21T05:45:17Z

Never mind that, found myself a clue

crypto/provider-core.c

crypto/provider.c

include/openssl/core-numbers.h

mattcaswell · 2019-02-21T11:06:43Z

Travis errors are relevant.

crypto/provider.c

levitte · 2019-02-21T12:06:31Z

Travis errors are relevant.

Yeah, I discovered that as well

levitte · 2019-02-21T17:38:29Z

The CIs finally approve :-)

levitte · 2019-02-21T20:21:46Z

Back in WIP, I forgot that I need to add docs as well

mspncp

I'm in doubt whether it is a good idea to introduce a new naming convention for C source code files by using the dash as a word separator. Traditionally, C source files have been in lowercase with underscores and the OpenSSL code base already has many of those files. Introducing a new separator would only make sense if it would have a different semantic meaning (like separating a package name from the module name) and only if it would be used throughout the entire project. Also, there should be a good reason for it, because the mixing of separators makes the code base look a bit untidy.

(Note that I am not talking about dashes in POD or Perl files)

levitte · 2019-02-22T08:57:05Z

Glad you picked up on that... personally, I find the use of underscore when not absolutely required (like in the limitations of symbol names) incredibly ugly.

Furthermore, it's interesting that you mention packaging in relation to this. See, in the Debian universe, underscores are used as delimiters in package file names, to separate between the package name (which can and frequently does include dashes), the package version, and the applicable architecture. Here are a couple of example from my cache:

libpandoc-wrapper-perl_0.9.0-1_all.deb
xserver-xorg-video-amdgpu_18.1.99+git20190207-1_amd64.deb

mspncp · 2019-02-22T09:06:03Z

Don't get me wrong: I'm not opposed to using the dash per se. (I love them in Lisp code ;-) ). I am just against ~~inconsistently~~ mixing separators inconsistently and without a reason.

The debian packages are actually a good example for having two separate kinds of separators, because in this case both separators serve different purposes. So if you can give a good justification of why and when to use dashes instead of underscores, and if this convention gets accepted by all other project members, that's ok for me. But just thinking that it looks nicer is not enough of a reason.

mspncp · 2019-02-22T09:07:50Z

If it were a new project, it would be ok (though uncommon) to use the dash throughout. But we already have a lot of files containing underscores.

levitte · 2019-02-22T09:18:35Z

Actually, we do use underscore as a sub-system / prefix delimiter, although not entirely consistently. In crypto/rsa, you'll see rsa_ everywhere. In crypto/evp, you will see encryption algorithm implementations fairly consistently prefixed with e_ and digest algorithm implementations fairly consistently prefixed with m_.

I have no intention to make "provider" a prefix or otherwise sub-system indicator. The only reason I have for two file names is that I wanted to separate the public API implementation from the internal API implementation. So "provider" and "provider-core" are just two names, with no other meaning intended.

levitte · 2019-02-22T09:19:51Z

However, if everyone else would prefer "provider_core" over "provider-core", I can change that. It's not like I'm very attached to either form.

mspncp · 2019-02-22T09:32:37Z

My preferences are clear, I think ;-)

levitte · 2019-02-22T10:10:55Z

Yeah, so maybe a second opinion, 'cause if it's left to just you and I, I fear we may get nowhere...

mspncp · 2019-02-22T10:24:41Z

That's ok for me. Since I braught up the subject, I'll ~~right (?!)~~ write a short note to openssl-project.

mattcaswell · 2019-02-22T13:38:45Z

I have a slight tendency to agree with @mspncp on the underscore vs dashes issue.

crypto/provider-core.c

crypto/provider.c

doc/internal/man3/ossl_provider_new.pod

include/internal/provider.h

mattcaswell

One minor issue noted. Approved whether or not a change is made in response to that.

mattcaswell · 2019-03-11T17:54:09Z

crypto/provider_core.c

+}
+
+/*
+ * Provider loader.


Should this comment really be talking about Provider activation?

levitte · 2019-03-11T19:41:05Z

Merged.

c453283 Add documentation
021a655 Add provider tests
16c2f1b Add provider module infrastructure
3374dc0 Replumbing: New public API to load or add providers
4c2883a Replumbing: Add the Provider Object, type OSSL_PROVIDER
3f4e8d6 Replumbing: Add MODULESDIR macro and OPENSSL_MODULES environment variable

…able These will be used to point out general OpenSSL modules directory. ENGINE modules are kept apart for backward compatibility. Reviewed-by: Matt Caswell <[email protected]> (Merged from #8287)

The OSSL_PROVIDER is the core object involved in loading a provider module, initialize a provider and do the initial communication of provider wide and core wide dispatch tables. Reviewed-by: Matt Caswell <[email protected]> (Merged from #8287)

Adding a provider means creating an internal provier object and adding it to the store. This allows the addition of built in providers, be it in the OpenSSL libraries or in any application. "Loading" a provider is defined broadly. A built in provider is already "loaded" in essence and only needs activating, while a provider in a dynamically loadable module requires actually loading the module itself. In this API, "loading" a provider does both. Reviewed-by: Matt Caswell <[email protected]> (Merged from #8287)

Reviewed-by: Matt Caswell <[email protected]> (Merged from #8287)

Two tests are added, one that tests the internal API, the other tests the public API. Those two tests both test the same provider, which acts both as a built-in provider and as a loadable provider module. Reviewed-by: Matt Caswell <[email protected]> (Merged from #8287)

Reviewed-by: Matt Caswell <[email protected]> (Merged from #8287)

featuring 6x"horizontal" code path which is up to 25% faster than present 4x"vertical" for larger blocks. Signed-off-by: Patrick Steuer <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #8287)

featuring 6x"horizontal" code path which is up to 25% faster than present 4x"vertical" for larger blocks. Signed-off-by: Patrick Steuer <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#8287) (cherry picked from commit d122919)

featuring 6x"horizontal" code path which is up to 25% faster than present 4x"vertical" for larger blocks. Signed-off-by: Patrick Steuer <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl/openssl#8287) (cherry picked from commit d1229190bfbb19439589557e4d65f9bccab09b2d) [Yilin: rebase babassl master] Signed-off-by: YiLin.Li <[email protected]>

levitte force-pushed the 300-provider-object branch 2 times, most recently from 5f22da9 to 38b9f0e Compare February 20, 2019 22:07

mattcaswell requested changes Feb 21, 2019

View reviewed changes

mattcaswell reviewed Feb 21, 2019

View reviewed changes

crypto/provider.c Outdated Show resolved Hide resolved

levitte force-pushed the 300-provider-object branch from 084a852 to 84cdd85 Compare February 21, 2019 13:06

levitte changed the title ~~WIP: Replumbing: Add the Provider Object, type OSSL_PROVIDER~~ Replumbing: Add the Provider Object, type OSSL_PROVIDER Feb 21, 2019

levitte changed the title ~~Replumbing: Add the Provider Object, type OSSL_PROVIDER~~ WIP: Replumbing: Add the Provider Object, type OSSL_PROVIDER Feb 21, 2019

mspncp previously requested changes Feb 22, 2019

View reviewed changes

levitte force-pushed the 300-provider-object branch from 6d869b8 to b704120 Compare February 22, 2019 10:11

levitte force-pushed the 300-provider-object branch 2 times, most recently from 59cd281 to ec42361 Compare February 22, 2019 11:38

mattcaswell requested changes Feb 22, 2019

View reviewed changes

fixup! Replumbing: New public API to load or add providers

6575445

mattcaswell approved these changes Mar 11, 2019

View reviewed changes

fixup! Replumbing: Add the Provider Object, type OSSL_PROVIDER

a9f2014

levitte closed this Mar 11, 2019

levitte added a commit that referenced this pull request Mar 11, 2019

Add provider module infrastructure

16c2f1b

Reviewed-by: Matt Caswell <[email protected]> (Merged from #8287)

levitte added a commit that referenced this pull request Mar 11, 2019

Add documentation

c453283

Reviewed-by: Matt Caswell <[email protected]> (Merged from #8287)

Uh oh!

Conversation

levitte commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

levitte commented Feb 19, 2019

Uh oh!

levitte commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Feb 20, 2019

Uh oh!

levitte commented Feb 21, 2019

Uh oh!

levitte commented Feb 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattcaswell commented Feb 21, 2019

Uh oh!

Uh oh!

levitte commented Feb 21, 2019

Uh oh!

levitte commented Feb 21, 2019

Uh oh!

levitte commented Feb 21, 2019

Uh oh!

mspncp left a comment

Choose a reason for hiding this comment

Uh oh!

levitte commented Feb 22, 2019

Uh oh!

mspncp commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mspncp commented Feb 22, 2019

Uh oh!

levitte commented Feb 22, 2019

Uh oh!

levitte commented Feb 22, 2019

Uh oh!

mspncp commented Feb 22, 2019

Uh oh!

levitte commented Feb 22, 2019

Uh oh!

mspncp commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell commented Feb 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

levitte Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

levitte commented Mar 11, 2019

Uh oh!

Reviewers

Assignees

levitte commented Feb 19, 2019 •

edited

Loading

levitte commented Feb 20, 2019 •

edited

Loading

mspncp commented Feb 22, 2019 •

edited

Loading

mspncp commented Feb 22, 2019 •

edited

Loading