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 header file docs for openssl/core_numbers.h and openssl/core_names.h #11963

Closed
wants to merge 5 commits into from

Conversation

@levitte
Copy link
Member

@levitte levitte commented May 26, 2020

No description provided.

@mspncp
Copy link
Contributor

@mspncp mspncp commented May 26, 2020

As a side note related to this pr: looking at the content of <openssl/core_numbers.h> I wonder whether the header name <openssl/core_functions.h> wouldn't be more appropriate? It's all about defining provider functions and the number, (i.e. the function ID) is just a part of their definition.

/* Memory allocation, freeing, clearing. */
#define OSSL_FUNC_CRYPTO_MALLOC 20
OSSL_CORE_MAKE_FUNC(void *,
CRYPTO_malloc, (size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_ZALLOC 21
OSSL_CORE_MAKE_FUNC(void *,
CRYPTO_zalloc, (size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_FREE 22
OSSL_CORE_MAKE_FUNC(void,
CRYPTO_free, (void *ptr, const char *file, int line))
#define OSSL_FUNC_CRYPTO_CLEAR_FREE 23
OSSL_CORE_MAKE_FUNC(void,
CRYPTO_clear_free, (void *ptr, size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_REALLOC 24
OSSL_CORE_MAKE_FUNC(void *,
CRYPTO_realloc, (void *addr, size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_CLEAR_REALLOC 25
OSSL_CORE_MAKE_FUNC(void *,
CRYPTO_clear_realloc, (void *addr, size_t old_num, size_t num,
const char *file, int line))
#define OSSL_FUNC_CRYPTO_SECURE_MALLOC 26
OSSL_CORE_MAKE_FUNC(void *,
CRYPTO_secure_malloc, (size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_SECURE_ZALLOC 27
OSSL_CORE_MAKE_FUNC(void *,
CRYPTO_secure_zalloc, (size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_SECURE_FREE 28
OSSL_CORE_MAKE_FUNC(void,
CRYPTO_secure_free, (void *ptr, const char *file, int line))
#define OSSL_FUNC_CRYPTO_SECURE_CLEAR_FREE 29
OSSL_CORE_MAKE_FUNC(void,
CRYPTO_secure_clear_free, (void *ptr, size_t num, const char *file,
int line))
#define OSSL_FUNC_CRYPTO_SECURE_ALLOCATED 30
OSSL_CORE_MAKE_FUNC(int,
CRYPTO_secure_allocated, (const void *ptr))
#define OSSL_FUNC_OPENSSL_CLEANSE 31
OSSL_CORE_MAKE_FUNC(void,
OPENSSL_cleanse, (void *ptr, size_t len))

Also, there are parts in the header where the IDs are intertwined with the function declarations, and other parts where the IDs precede the declarations blockwise.

/* Bio functions provided by the core */
#define OSSL_FUNC_BIO_NEW_FILE 40
#define OSSL_FUNC_BIO_NEW_MEMBUF 41
#define OSSL_FUNC_BIO_READ_EX 42
#define OSSL_FUNC_BIO_WRITE_EX 43
#define OSSL_FUNC_BIO_FREE 44
#define OSSL_FUNC_BIO_VPRINTF 45
#define OSSL_FUNC_BIO_VSNPRINTF 46
OSSL_CORE_MAKE_FUNC(OSSL_CORE_BIO *, BIO_new_file, (const char *filename,
const char *mode))
OSSL_CORE_MAKE_FUNC(OSSL_CORE_BIO *, BIO_new_membuf, (const void *buf, int len))
OSSL_CORE_MAKE_FUNC(int, BIO_read_ex, (OSSL_CORE_BIO *bio, void *data,
size_t data_len, size_t *bytes_read))
OSSL_CORE_MAKE_FUNC(int, BIO_write_ex, (OSSL_CORE_BIO *bio, const void *data,
size_t data_len, size_t *written))
OSSL_CORE_MAKE_FUNC(int, BIO_free, (OSSL_CORE_BIO *bio))
OSSL_CORE_MAKE_FUNC(int, BIO_vprintf, (OSSL_CORE_BIO *bio, const char *format,
va_list args))
OSSL_CORE_MAKE_FUNC(int, BIO_vsnprintf,
(char *buf, size_t n, const char *fmt, va_list args))

I find the latter variant more readable. Whichever variant you choose, it should be consistent.

@Tymbur
Copy link

@Tymbur Tymbur commented May 26, 2020

@mspncp
Copy link
Contributor

@mspncp mspncp commented May 26, 2020

@Tymbur I have no idea why you received the mail. It was a regular post of mine on pull request #11963. To prevent further email notifications, you can follow the link to the pull request and unsubscribe from the thread.

@Tymbur
Copy link

@Tymbur Tymbur commented May 26, 2020

@levitte
Copy link
Member Author

@levitte levitte commented May 27, 2020

As a side note related to this pr: looking at the content of <openssl/core_numbers.h> I wonder whether the header name <openssl/core_functions.h> wouldn't be more appropriate?

Not sure that's the best replacement names either, the name "core_functions" would make me expect to see a number of declarations for functions offered by the core of libcrypto... regarding the name "core_numbers", I would argue that these are dispatch numbers offered by the core, along with the function types that the provider need to associate with those numbers.

@levitte
Copy link
Member Author

@levitte levitte commented May 27, 2020

Also, there are parts in the header where the IDs are intertwined with the function declarations, and other parts where the IDs precede the declarations blockwise.
...
I find the latter variant more readable. Whichever variant you choose, it should be consistent.

I would say that it depends. The functions offered by the core (!!!) is a looong list, that's a lot of jumping back and forth to see what function number macro corresponds to what function signature.
Although, one might argue that some operations have a long list of numbers/function signatures as well, where we separate the number block from the function signature block, so we're not quite consistent there.

@levitte
Copy link
Member Author

@levitte levitte commented May 27, 2020

Now that I've commented on stuff that isn't about this PR itself at all, anyone want to review?

@slontis
Copy link
Contributor

@slontis slontis commented May 28, 2020

There are some make doc-nits errors in travis.

Copy link
Contributor

@slontis slontis left a comment

Assuming tests all pass.

@slontis slontis added this to Reviewer approved in 3.0 New Core + FIPS via automation May 28, 2020
A CAVEATS section is present in this manual.  That section name is
borrowed from OpenBSD, where mdoc(7) explains it like this:

    CAVEATS
    Common misuses and misunderstandings should be explained in this
    section.
@levitte
Copy link
Member Author

@levitte levitte commented May 28, 2020

Care to re-approve with the squash commit I just added?

Copy link
Contributor

@slontis slontis left a comment

Reapproved

@richsalz
Copy link
Contributor

@richsalz richsalz commented May 28, 2020

I dislike adding a new CAVEATS section. OpenSSL has always used NOTES, which seems quite appropriate here.

Don't blaze new trails. Be boring. Be consistent.

@levitte
Copy link
Member Author

@levitte levitte commented May 28, 2020

You often want to collapse NOTES into DESCRIPTION too... 😉

I think this particular one does deserve a bit more "voice" than a mere note...

@richsalz
Copy link
Contributor

@richsalz richsalz commented May 28, 2020

Yes, I often collapse NOTES into DESCRIPTION because it doesn't deserve a separate call-out, and folks get lazy by just adding NOTES after the main manpage is written.

Look at other notes sections. This is no different.

I'm sure I'll lose this point, sigh, but you're still wrong.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented May 29, 2020

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@levitte
Copy link
Member Author

@levitte levitte commented May 29, 2020

Merged.

329b2a2 DOCS: add openssl-core_numbers.h(7)
f438f53 DOCS: add openssl-core_names.h(7)

@levitte levitte closed this May 29, 2020
3.0 New Core + FIPS automation moved this from Reviewer approved to Done May 29, 2020
openssl-machine pushed a commit that referenced this pull request May 29, 2020
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #11963)
openssl-machine pushed a commit that referenced this pull request May 29, 2020
A CAVEATS section is present in this manual.  That section name is
borrowed from OpenBSD, where mdoc(7) explains it like this:

    CAVEATS
    Common misuses and misunderstandings should be explained in this
    section.

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #11963)
@levitte levitte deleted the levitte:add-header-file-docs-20200526 branch May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants