Include common.h from all library source files #3454

gilles-peskine-arm · 2020-06-24T13:44:08Z

Include common.h from library/*.c.

This is a precursor to doing useful things with it, such as declaring macros related to test hooks, or things that we would have put in platform.h for lack of a better place. For the time being, it just contains the universal idiom for including the config file.

This should not be backported because we don't add new files in LTS branches. Anything that gets added to common.h will need to be added or kept elsewhere in LTS branches.

mpg

LGTM

When compiling library files under `3rdparty/`, the directory containing the `.c` file that is being compiled is not the current directory, so headers from the `library/` directory are not found. Fix this by adding `.` to the include path. This was not detected until now because as of this commit, no 3rdparty source file requires a header under `library/`. Signed-off-by: Gilles Peskine <[email protected]>

In library source files, include "common.h", which takes care of including "mbedtls/config.h" (or the alternative MBEDTLS_CONFIG_FILE) and other things that are used throughout the library. FROM=$'#if !defined(MBEDTLS_CONFIG_FILE)\n#include "mbedtls/config.h"\n#else\n#include MBEDTLS_CONFIG_FILE\n#endif' perl -i -0777 -pe 's~\Q$ENV{FROM}~#include "common.h"~' library/*.c 3rdparty/*/library/*.c scripts/data_files/error.fmt scripts/data_files/version_features.fmt Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm · 2020-07-02T09:30:20Z

I rebased due to a merge conflict (in an automatically generated file, but that would still have prevented an automatic merge). The result is identical to merging development (at 82ac38e) with the previous version of this PR (https://github.com/gilles-peskine-arm/mbedtls/tree/include-common-h-development-4).

mpg

Re-approving after checking that the result of the rebase is indeed equivalent to a merge of development into the version I previously approved.

ronald-cron-arm · 2020-07-02T09:57:39Z

Because of the recent changes on CMake build system, you will probably need to do the same kind of change you've done to the make build system to the CMake build system.

ronald-cron-arm · 2020-07-02T11:05:55Z

By the way I can do the change to the CMake build system if you prefer.

All libraries (should) rely on the same directory structure. Instead of repeating the same clauses 6 times (3 libraries times 2 build modes), set the include paths, compile definitions and install instructions with a single piece of code. Include the 3rdparty directory for all libraries, not just crypto. It's currently only needed for crypto, but that's just happenstance. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm · 2020-07-02T11:10:15Z

I'm working on the cmake change.

"Include the library directory for the sake of 3rdparty" did the job for Make and Visual Studio. This commit does the job for CMake. Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm · 2020-07-02T11:20:57Z

CMake update. @ronald-cron-arm (and CI) please let me know if that's not the right way to do it.

ronald-cron-arm

I have a question not directly on this PR but related. I am wondering if it has been considered to put those library restricted headers in a dedicated folder? There are already a fair amount of files in ./library with a mix of *.c and .h files thus ordering those files a bit more would make sense to me. Furthermore if we put the header files into an ./library/internal folder for example, the pattern to include the headers could be made to be internal/.h making it explicit that this is a private/internal header file that is included.

library/CMakeLists.txt

gilles-peskine-arm · 2020-07-02T11:42:56Z

if it has been considered to put those library restricted headers in a dedicated folder?

I'm the one who introduced .h files in library and nobody really questioned it AFAIR. My reasoning is that these header files are only needed when building the library (and invasive tests), so they belong in the same location as the library's .c files. I don't see the point of a separate directory. It wouldn't hurt, other than a small amount of additional complexity in build scripts (which we already mostly have anyway due to the existence of 3rdparty), but I don't see any benefit.

ronald-cron-arm · 2020-07-02T12:25:32Z

I've explained two benefits I see but I acknowledge that those are not definitive arguments. It would look better to me to see something like <internal/common.h> or <mbedtls-internal/common.h> than just <common.h>. The mbedtls private/internal nature of the header would be explicit and this would align with the mbedtls/.h, psa/.h, test/*.h ways of including headers.

Signed-off-by: Gilles Peskine <[email protected]>

library/CMakeLists.txt

Signed-off-by: Gilles Peskine <[email protected]>

ronald-cron-arm

LGTM

mpg

LGTM

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts labels Jun 24, 2020

gilles-peskine-arm mentioned this pull request Jun 24, 2020

Remove unnecessary & 0xFF #3441

Closed

gilles-peskine-arm force-pushed the include-common-h-development branch from d54ea42 to ceb8327 Compare June 24, 2020 14:42

danh-arm requested a review from mpg June 25, 2020 09:25

gilles-peskine-arm mentioned this pull request Jun 25, 2020

Document clearly that 16-bit systems are not supported #3323

Closed

mpg previously approved these changes Jun 26, 2020

View reviewed changes

gilles-peskine-arm added 2 commits July 2, 2020 11:26

gilles-peskine-arm dismissed mpg’s stale review via db09ef6 July 2, 2020 09:29

gilles-peskine-arm force-pushed the include-common-h-development branch from ceb8327 to db09ef6 Compare July 2, 2020 09:29

gilles-peskine-arm requested a review from mpg July 2, 2020 09:30

mpg previously approved these changes Jul 2, 2020

View reviewed changes

gilles-peskine-arm added 2 commits July 2, 2020 13:13

CMake: Include the library directory for the sake of 3rdparty

dedff7a

"Include the library directory for the sake of 3rdparty" did the job for Make and Visual Studio. This commit does the job for CMake. Signed-off-by: Gilles Peskine <[email protected]>

Library files aren't supposed to be executable

280165c

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm dismissed mpg’s stale review via 280165c July 2, 2020 11:19

gilles-peskine-arm added the needs-ci Needs to pass CI tests label Jul 2, 2020

ronald-cron-arm reviewed Jul 2, 2020

View reviewed changes

ronald-cron-arm requested changes Jul 2, 2020

View reviewed changes

library/CMakeLists.txt Show resolved Hide resolved

Add comments explaining include paths

76dd3aa

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm requested review from ronald-cron-arm and mpg July 2, 2020 14:01

gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Jul 2, 2020

ronald-cron-arm requested changes Jul 2, 2020

View reviewed changes

library/CMakeLists.txt Show resolved Hide resolved

library/CMakeLists.txt Show resolved Hide resolved

library/CMakeLists.txt Outdated Show resolved Hide resolved

Remove redundant assignment

e1c4362

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm requested a review from ronald-cron-arm July 2, 2020 17:48

ronald-cron-arm approved these changes Jul 3, 2020

View reviewed changes

mpg approved these changes Jul 3, 2020

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jul 3, 2020

mpg merged commit 527b878 into Mbed-TLS:development Jul 3, 2020

tommikas mentioned this pull request Apr 26, 2021

Header name is too common in #include "common.h" #4425

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include common.h from all library source files #3454

Include common.h from all library source files #3454

gilles-peskine-arm commented Jun 24, 2020

mpg left a comment

gilles-peskine-arm commented Jul 2, 2020

mpg left a comment •

edited

Loading

ronald-cron-arm commented Jul 2, 2020 •

edited

Loading

ronald-cron-arm commented Jul 2, 2020 •

edited

Loading

gilles-peskine-arm commented Jul 2, 2020

gilles-peskine-arm commented Jul 2, 2020

ronald-cron-arm left a comment

gilles-peskine-arm commented Jul 2, 2020

ronald-cron-arm commented Jul 2, 2020

ronald-cron-arm left a comment

mpg left a comment

Include common.h from all library source files #3454

Include common.h from all library source files #3454

Conversation

gilles-peskine-arm commented Jun 24, 2020

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jul 2, 2020

mpg left a comment • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm commented Jul 2, 2020 • edited Loading

ronald-cron-arm commented Jul 2, 2020 • edited Loading

gilles-peskine-arm commented Jul 2, 2020

gilles-peskine-arm commented Jul 2, 2020

ronald-cron-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jul 2, 2020

ronald-cron-arm commented Jul 2, 2020

ronald-cron-arm left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment •

edited

Loading

ronald-cron-arm commented Jul 2, 2020 •

edited

Loading

ronald-cron-arm commented Jul 2, 2020 •

edited

Loading