genodelabs / genode

Genode OS Framework
https://genode.org/
Other
1.02k stars 249 forks source link

tresor: remove module framework #5148

Closed m-stein closed 2 weeks ago

m-stein commented 1 month ago

In the context of this issue, several suggestions for improving the result of the last Tresor clean-up (https://github.com/genodelabs/genode/issues/5062) are applied. The main points are:

m-stein commented 1 month ago

This would be my first proposal for review: d43076b0ed tresor: remove module framework 229c79b553 tresor_utils.run: use [build_artifacts] 7b80adad7d tresor: use / .. / instead of // .. 43cb5ef6c3 tresor: use (..) instead of { .. } in init lists

m-stein commented 1 month ago

@chelmuth I tried to compress my merge_to_staging branch. If you opt for more compression, please let me know!

chelmuth commented 1 month ago

The merge revealed (maybe 32-bit related) hiccups in the following depot packages.

The builder confirms this with errors on arm_v6, arm_v7a, and x86_32.

chelmuth commented 1 month ago

run/tresor_tester fails on all platforms but linux.

[init] child "block_io_fs" requests resources: ram_quota=2101248

x86_64.pc.nova.file_vault_client

Line 9 of output is unexpected
expected: '[init -> dynamic_init -> log_terminal] Und Sonderzeichen: /§($)=%!'
got:      '[init -> dynamic_init -> log_terminal] Und Sonderzeichen: /..($)=%!'
Error: Test failed, 1 unexpected lines of output

arm_v8a.imx8q_evk.hw.file_vault_client

Line 3 of output is unexpected
expected: '[init -> dynamic_init -> log_terminal] -rwx------ 1 root 0 18 Jan  1  1970 file_1'
got:      '[init -> dynamic_init -> log_terminal] -rw------- 1 root 0 18 Jan  1  1970 file_1'

Line 9 of output is unexpected
chelmuth commented 1 month ago

@m-stein please address the errors above with priority, thanks.

m-stein commented 1 month ago

@chelmuth I'm at it. Thanks for the pointers!

m-stein commented 1 month ago

@chelmuth I started with re-enabling support for x86_32 (testing with nova) and found it sensible to introduce a new type Number_of_disk_bytes, because many places that used to use addr_t, size_t or something related actually referred to addresses, sizes, etc. on either the "virtual" (virtual block device) or "physical" (block back-end) block device. I think that these address spaces should be assumed to have 64-bit addresses independently from the target platform. Here's the commit that could be merged already: 7bac2ffafb.

I will continue with the ARM platforms.

m-stein commented 1 month ago

@chelmuth My arm_v7a and arm_v6 tests succeeded as well with the above commit in place.

m-stein commented 1 month ago

@chelmuth I couldn't reproduce the nova error in qemu. I can investigate this further in office if you opt for it or we just remove the apparently troublesome § character for now: 24e1e1615f

m-stein commented 1 month ago

@chelmuth This should fix the file-mode issue on arm_v8: 041ab6320f

chelmuth commented 1 month ago

@chelmuth I started with re-enabling support for x86_32 (testing with nova) and found it sensible to introduce a new type Number_of_disk_bytes, because many places that used to use addr_t, size_t or something related actually referred to addresses, sizes, etc. on either the "virtual" (virtual block device) or "physical" (block back-end) block device. I think that these address spaces should be assumed to have 64-bit addresses independently from the target platform. Here's the commit that could be merged already: 7bac2ff.

Currently, I'd appreciate just a fix of the problem at hand: Don't use addr_t and size_t where uint64_t should be used. Your intention to clean up and tighten the implementation by means of stronger typing is well-meant but your implementation as plain typedef to uint64_t does not address your intention. Therefore please, let's only fix the build errors for now. We may discuss clean-up options offline in the next meeting.

m-stein commented 1 month ago

@chelmuth Thanks for your feedback! This is a less invasive approach: 47cb1cd813 run/file_vault_client: consider varying file modes 2fcb541960 run/file_vault_client: remove troublesome § char ab50b9def5 file_vault: re-enable support for 32-bit platforms

chelmuth commented 1 month ago

47cb1cd run/file_vault_client: consider varying file modes

What is the background of the variance on arm_v8a/arm_v7a in comparison to x86 and arm_v6?

2fcb541 run/file_vault_client: remove troublesome § char ab50b9d file_vault: re-enable support for 32-bit platforms

Merged to staging.

m-stein commented 1 month ago

@chelmuth Good that you asked! Josef and I apparently found a bug in VFS rump that caused the different modes: d4cc70b3c2 file_vault_client.run: fix expected file modes 19b5e661bc vfs_rump: fix missing create arg in open af1aff7506 run/file_vault_client: remove troublesome § char ab50b9def5 file_vault: re-enable support for 32-bit platforms

chelmuth commented 1 month ago

Merged 337e0ecae857f1a4cc9a4fe88b786ce8bdc5de1d, 583e5bee0589fc3c1dce5754c7dd5ff98fc27ef7, and 8adb297903097132dc787c63eec601fc0d3b155d as fixup. Do you approve?

m-stein commented 1 month ago

@chelmuth Yes, thank you!

m-stein commented 3 weeks ago

@chelmuth With these commits, tresor_tester should succeed on all platforms and file_vault_client should fail on less platforms: 6f0b211612 file_vault: raise child resources e7fd3f1d58 file_vault_client.run: raise access timeout 77490c9312 file_vault_client.run: disable for riscv 02610d305c tresor_tester.run: raise block_io_fs caps 74e87de503 tresor_tester.run: raise test timeout

There are some fails with file_vault_client that I couldn't reproduce yet (for instance qemu/x86_32/fiasco). I assumed that it is related to the used qemu version (I use 6.1). However, I couldn't figure out which version is used in the nightly tests because my login failed and therefore tried some older qemu versions (4,4.2,5,5.2) but without a result.

m-stein commented 3 weeks ago

@chelmuth Ok, it seems, that the remaining fails where also timeout-related and I just lost myself in the logs. I noticed that testing with qemu on the test machine is significantly slower than on my machine, presumably because of ´´-no-kvm´´.

cnuke commented 3 weeks ago

Commit bb7d61d changes the default permissions to 0777 (cover +x as it is not possible to set it otherwise) to prevent regressions, which should make file_vault_client.run: fix expected file modes dispensable.

nfeske commented 3 weeks ago

Thank you for the fixup. I merged it to staging and reverted the file_vault_client.run commit.

m-stein commented 2 weeks ago

@chelmuth This commit should fix most of the remaining fails with file_vault_client: 14ecd4db34 file_vault_client.run: raise lock timeout and caps

chelmuth commented 2 weeks ago

Tresor rework entered the master branch.