Add client-server build to all.sh #9121
Conversation
…and server It includes changes to: - tests/Makefile: build the library for client and server in different folders. It mimica the libtestdriver1 behavior (without functions renaming though). - tests/scripts/all.sh: helper function to build for client and server with some default configuration for each of them. - crypto_spe.h: this is dummy file taken from the already existing tests. It's just meant to pacify the compiler, not to provide something useful. It will likely be changed in the future. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
…er MbedTLS libraries Ensure that both server and client can call mbedtls_version_get_string_full() to verify that they are linked against proper libraries. Note: each side (client/server) performs the call against its own MbedTLS library. There is no IPC communication involved in this test. Client/server communication will come later. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
This commit implements the first useful IPC communication between the client and the server. The implemented command is simple, psa_crypto_init(), and its return value is sent back to the client. Note: the newly added file psa_functions_codes.h is temporary and it's probably the one that needs to be automatically generated by a python script to support all crypto functions. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
This is necessary because otherwise the library is not able to find the seedfile at runtime and it fails the initialization. However since this test runs on a standard PC we can rely on platform entropy as source of entropy. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
valeriosetti
added
needs-review
The goal is to keep psasim as simple as possible: - do not build a separate lib for psa-ff; build those source files as part of server or client - do not have lot of different makefiles: just 1 that does all we need - do not have several subfolders for headers: only 1 is enough for this kind of project Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
tests/Makefile
Outdated
|
|
||
| libpsaclient libpsaserver: | ||
| # Clone the library and include folder for client and server builds. | ||
| rm -Rf ./$@ |
There was a problem hiding this comment.
(minor) consistency: why not use -rf like at like 176? (Yes, I'm aware we sometimes seem to use -r and sometimes -R for no very good reason)
|
|
||
| ifeq ($(DEBUG),1) | ||
| CFLAGS += -DDEBUG -O0 -g | ||
| CFLAGS += -DDEBUG |
There was a problem hiding this comment.
Why remove -O0 and -g - if we are going to debug, they are very useful options to have built the executables with!
There was a problem hiding this comment.
I know I know, I've used them thousand times in the past, but those are passed in by all.sh. I usually go to all.sh and modify ASAN_FLAGS replacing -O2 with -O0 -g.
There was a problem hiding this comment.
Yes, but this is specifically for a DEBUG build (the test on line 3). Why require a developer to modify a file when these can just be kept in?
There was a problem hiding this comment.
I was afraid that adding extra -O0 -g on a line which already contained -O2 (from ASAN_FLAGS) could not work properly, but googling a bit turned out that it actually does what we expect. I fixed this in c98f8ab
Instead of copying the entire library & include folders twice to build libraries for client and server: - change the main config file (mbedtls_config.h) - build in the root library folder - move the generated library in the psasim folder - use those library for linking the client/server binaries Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Add DEBUG=1 in test_psasim() to helpers and final make to build the libraries and the final binaries with debug symbols enabled. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
tests/scripts/all.sh
Outdated
| TARGET=$1 | ||
| TARGET_LIB=libpsa$TARGET | ||
|
|
||
| cp $CONFIG_H $CONFIG_H.bak |
There was a problem hiding this comment.
It this not done by the pre_back_up ref 1 and 2?
What happens if make command fails, or if the config file is the driver config?
There was a problem hiding this comment.
Yes, but pre_back_up dumps files which are restore at the end of run_component. In our case, however, we are modifying config files twice in the same component. So unless I missed something here, we need this extra logic. Wdyt?
What happens if make command fails
Right, we might end up with a modified config file which can erroneously be picked up as the valid one on the next run (unless the user realizes that and clears the status with git restore ...). I will fix this by creating the "new/custom" config files as copy of the original ones so that if something fails in during the build, then those files will be overwritten by a new all.sh run.
if the config file is the driver config
Mmmm right, I didn't thought about this but I need to make a copy also of this file! Thanks for the notification
There was a problem hiding this comment.
Update: while implementing what I wrote above I realized that this was making helper_crypto_client_build more complex for probably no reason. So I ended up not copying config files, just relying on the backed-up ones, and then calling cleanup in the end. The final shape is f57afd5. Wdyt?
| INCLUDE := -I../include/ -I./psa_manifest | ||
| LIB := -L../src -lpsaff | ||
| LIBPSASIM_PATH := .. | ||
| LIBPSACLIENT_PATH := ../../../libpsaclient |
There was a problem hiding this comment.
could we put the root path e.g ../../.. into a variable so when the library moves it will be simpler?
| invec = 0; | ||
| outvec = 0; | ||
|
|
||
| // read response from server |
There was a problem hiding this comment.
Not a blocker but could we use consistent comments? .e.g /* comment */
|
|
||
| static int get_queue_info(char *path, int *cqid, int *sqid) | ||
| { | ||
|
|
There was a problem hiding this comment.
(minor) unnecessary blank line at the beginning of this function and the next
| static psa_status_t send(int rx_qid, int server_qid, int *internal_server_qid, | ||
| int32_t type, uint32_t minor_version, vectors_t *vecs) | ||
| { | ||
| { |
There was a problem hiding this comment.
(minor) unnecessary extra level of blocking
|
|
||
| psa_handle_t psa_connect(uint32_t sid, uint32_t minor_version) | ||
| { | ||
|
|
There was a problem hiding this comment.
(minor) blank line
| psa_outvec *out_vec, | ||
| size_t out_len) | ||
| { | ||
|
|
There was a problem hiding this comment.
(minor) blank line
- add quotes to the $@ parameter in helper_crypto_client_build() - instead of copying mbedtls_config.h to build static libraries, we rely on the already existing backup/cleanup mechanism which is available in all.sh. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
|
[Personal opinion] |
This allows to re-enable MBEDTLS_ENTROPY_NV_SEED since the seedfile is correctly found in the "test" folder at runtime. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
valeriosetti
dismissed
minosgalanakis’s stale review
The merge-base changed after approval.
| @@ -15,6 +15,9 @@ | |||
|
|
|||
| FILENAME = str(sys.argv[1]) | |||
|
|
|||
| SCRIPT_PATH = os.path.dirname(__file__) | |||
There was a problem hiding this comment.
Small side-note. We have a tool that allows you to find the absolute path.
from mbedtls_dev import build_tree
root_dir = build_tree.guess_mbedtls_root()
There was a problem hiding this comment.
Right, I've used it recently in another PR. However in this case I think it is better to keep a relative path instead of an absolute one (relative in this case means local to the psasim project/folder, while absolute means something related to the MbedTLS root folder). It is more portable in my opinion in case in the future we move the psasim project to the framework folder/repo. Wdyt?
| sids = open("psa_manifest/sid.h", "a") | ||
| man = open(os.path.join(GENERATED_H_PATH, FILENAME + ".h"), "w") | ||
| pids = open(os.path.join(GENERATED_H_PATH, "pid.h"), "a") | ||
| sids = open(os.path.join(GENERATED_H_PATH, "sid.h"), "a") |
There was a problem hiding this comment.
question: would it make sense to create a function open_in_include_dir() to avoid the repetition of the os.path.joins?
| @@ -19,6 +19,9 @@ | |||
| GENERATED_H_PATH = os.path.join(SCRIPT_PATH, "..", "include", "psa_manifest") | |||
| GENERATED_C_PATH = os.path.join(SCRIPT_PATH, "..", "src") | |||
|
|
|||
| def open_in_include_dir(filename: str): | |||
| return open(os.path.join(GENERATED_H_PATH, filename), "w") | |||
There was a problem hiding this comment.
I am not a fan of this syntax, since it will not close the file handler. I would remove this function and use nested with statements instead,
with open(os.path.join(GENERATED_H_PATH, "pid.h"), "w"):
with open(os.path.join(GENERATED_H_PATH, "sid.h"), "w"):
with open(os.path.join(GENERATED_H_PATH, FILENAME + ".h"), "w"):
...other logic....
Alternatively we can use striongIO objects to compile our files and then write them into the headers.
There was a problem hiding this comment.
IIUC you don't need to nest, but can separate open()s with just ,
There was a problem hiding this comment.
Ok, but those files are opened inside another with open() block (for reading the input file) and they are written to in several lines along the script. Following this approach means that a good part of the script would be right indented a lot. Don't get me wrong, I agree with you that what you are proposing is the right way of writing to a file in a python script, but I'm only stating that this change basically requires/suggests to heavily modify that script.
There was a problem hiding this comment.
We can keep the open_in_include_dir function, but it should be used like open, i.e.
with open_in_include_dir('foo.h') as foo:
... foo.read() ...
a good part of the script would be right indented a lot
That's a hint to break the code into more auxiliary functions/methods.
| sids.close() | ||
| man.close() | ||
| with open(MANIFEST_FILE, "wt") as output: | ||
| output.write("".join(manifest_content)) |
There was a problem hiding this comment.
Should that be output.write("\n".join(manifest_content)) ?
Don' t we need each of those entries to be on its' own line?
Same with SID and PID files bellow.
There was a problem hiding this comment.
Should that be output.write("\n".join(manifest_content)) ?
It actually works because I just replaced sid.write() with sid_content.append(), but I didn't change the content of the parenthesis, so what was generated before it will be also now. However following Tom's suggestion I can remove new lines from the parenthesis so later there will be "".join() as you suggested.
While at this, fix also Makefile so that "make clean" does not complain if some of the files to be cancelled do not exist. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
| C_FILENAME = os.path.join(GENERATED_C_PATH, "psa_ff_bootstrap_" + partition_name + ".c") | ||
| c_content = [] | ||
| c_content.append("#include <init.h>") | ||
| c_content.append("#include \"" + symbols[0] + "\"") |
There was a problem hiding this comment.
or
print(f'#include "{symbols[0]}"')
There was a problem hiding this comment.
Just a remark, not an objection (quite the contrary): f-strings are a new feature in Python 3.6. We can use Python 3.6 constructs only since last week and this would be our first non-3.6-compatible code.
tom-cosgrove-arm
added
approved
|
@tom-cosgrove-arm I just noticed that you added both the |
Description
This PR extends the
psasimtesting as follows:crypto,x509andtls) twice, one for the client and one for the server with proper configurations. Most relevant difference is:CRYPTO_CLIENT && !CRYPTO_Cand links all 3 librariescrypto,x509andtls;CRYPTO_Cand only links againstcryptolibrary.psa_crypto_init(). This will be extended in the futureResolves #8963
PR checklist
psasimis a new componentpsasimis a new component