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

SSL_ part in macro names #9858

Open
irwir opened this issue Dec 17, 2024 · 6 comments
Open

SSL_ part in macro names #9858

irwir opened this issue Dec 17, 2024 · 6 comments

Comments

@irwir
Copy link
Contributor

irwir commented Dec 17, 2024

Does it make sense to keep SSL_ part in macro names if there is already MBEDTLS_ prefix?

#define MBEDTLS_SSL_ALL_ALERT_MESSAGES

This TLS 1.3 feature was not expected to be a part of SSL.

#define MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_ENABLED

A few names could be made shorter by avoiding tautological repetitions, and now it is a rare time for such changes - next major version release might be quite far away.

@yanesca
Copy link
Contributor

yanesca commented Dec 17, 2024

It might seem redundant, but it serves a purpose. MBEDTLS_ is prefix for the Mbed TLS project which includes 3 libraries: libmbedcrypto, libmbedtls and libmbedx509. The SSL_ prefix identifies the library.

(In 4.0 there will only be libmbedtls and libmbedx509, but that is still two libraries and it justifies a prefix for each.)

@irwir
Copy link
Contributor Author

irwir commented Dec 18, 2024

Interesting.
Consistent naming usually is a hard target; even more so for code with longer history.

@irwir
Copy link
Contributor Author

irwir commented Dec 19, 2024

(In 4.0 there will only be libmbedtls and libmbedx509, but that is still two libraries and it justifies a prefix for each.)

Trying to minimize things, MBEDTLS_ and MBEDTLS_X509 could be guessed.
Should MBEDTLS_ASN1_PARSE_C become MBEDTLS_X509_ASN1_PARSE_C?

@yanesca
Copy link
Contributor

yanesca commented Dec 20, 2024

Trying to minimize things, MBEDTLS_ and MBEDTLS_X509 could be guessed.

We normally try to minimize the thought that the reader has to put in understanding the code. I agree, that length can be at odds with that goal, but I don't feel that we are there yet in this case.

Should MBEDTLS_ASN1_PARSE_C become MBEDTLS_X509_ASN1_PARSE_C?

4.0 won't have it's own MBEDTLS_ASN1_PARSE_C, it will be pulled in from TF-PSA-Crypto as part of the submodule.

@irwir
Copy link
Contributor Author

irwir commented Dec 20, 2024

I agree, that length can be at odds with that goal, but I don't feel that we are there yet in this case.

Macro MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL_ENABLED has 58 characters in 10 parts (words) - not too short phrase taking most of an 80-characters line.
It very well could be already "there".

@yanesca
Copy link
Contributor

yanesca commented Dec 20, 2024

That is indeed very long. A mitigating factor is that our new coding standard allows 100 characters, but still, I see your point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants