SUIT: Prepared manifests

[chrysn]

Contribution description

While SUIT can generally be used already with manifests that are "dropped into memory" (by any mechanism), the convenient SUIT worker thread mechanism so far could not be used with it.

This adds the suit_worker_try_prepare / suit_worker_trigger_prepared
pair for using the SUIT worker, and breaks suit_handle_manifest_buf out
of suit_handle_url (where the latter now calls the former).

By-catch

As part of factoring out reaping of the zombie worker thread, a locking
error that deadlocks the SUIT worker in case the race between the mutex
being unlocked and the thread being reaped hits the necessary handling
code is fixed (and mutex_unlock is called in the error path).

Testing procedure

SUIT tests should pass.

#19659 provides a demo of how the new API is used.

Issues/PRs references

I think that the currently employed mechanism of having a resource to which a URL with the manifest gets posted is contrary to the design goals of SUIT -- the signed manifest should be what justifies the device to spend resources (eg. get data from a server), not a URI that is just not signed.

This PR makes it easier to implement a resource to which the manifest can be POSTed, rather than a CoAP URI that represents the manifest, as is proposed (but not mature) in
#19659.

[edit: Adjusted to reflect decisions made during review]

[chrysn]

I'm not fully happy with the write lock situation -- but the low-level functions are already a bunch of mutually exclusive user-needs-to-coordinate functions. Maybe I'll still replace saul_worker_is_idle with an acquire/release pair that's needed around writes to the manifest buffer.

[chrysn]

... and updated accordingly. While the low-level functions are wild as they were before, the worker..prepare family is now an acquire/commit pair (with the commit doubling as a panic hatch), which is easier to understand and use.

[riot-ci]

Murdock results

✔️ PASSED

8d6a375 sys/suit: Fix missing include

Success	Failures	Total	Runtime
6933	0	6933	11m:36s

Artifacts

• Documentation preview

[chrysn]

CI tests briefly disabled; I'd like to rebase it onto #19643 later to ease testing.

[chrysn]

I've pushed this onto current master, as that allows running the tests again after #19643 was fixed.

Also, there is now #19659, which makes this testable more practically.

[chrysn]

Right now this (should) behave like suit_worker_trigger, just in two steps (prepare, trigger_prepared) instead of one.

I'm happy to change things around so that we get completion indication, but I'd prefer to do that separately and not while introducing a new feature, and then consistently across both prepared and by-URI manifests.

[benpicco]

I don't quit understand why this is needed - also shouldn't we then unlock the mutex again?

[chrysn]

That lock has always been in suit_worker_trigger before as well -- things were just split because suit_worker_trigger acquires the lock, whereas suit_worker_trigger_prepared already had it acquired in suit_worker_prepare (which, given its differing locking behavior, I've just renamed for consistency to suit_worker_try_prepare).

The worker lock is there to cover both access to the manifest buffer and to the thread. The lock is released by the thread. Therefore, the release of the lock just indicates that the thread is almost gone, not that it's gone for real -- that can only be made sure of by reaping the zombie state process (which is the thing we can't await for lack of thread_join functionality).

You're right on the unlocking part, that was a bug already -- it's fixed now and better documented; the complexity of it indicates that it might be a good idea to follow @kaspar030's suggestion of using a long-running thread (idempotently started on first use to retain the nice gc-sections/gc-functions properties) rather than struggling with locks around a thread stack.

[benpicco]

I agree that the complexity of the locking has grown pretty hairy - when the mutex is locked in suit_worker_try_prepare() it is then unlocked either in suit_worker_trigger_prepared(), _worker_reap() or _suit_worker_thread().

That's downright scary, but I think this will work correctly - please squash.

[chrysn]

Changes after approval were limited to typo corrections (35d5332), a missing semicolon and missing rename (66baee6), and the still-independent-because-its-a-standalone-thing include fix of c047331 (which is what broke the tests as in those the C file's own header that was always formally missing but not practically relevant became relevant during these changes, and the test's setup didn't do the same includes that my code did that made it not matter).

[chrysn]

Fixed a final issue where an argument was only used in side an assert (so building with NDEBUG as in DEVELHELP=0 didn't use the variable) in 8d6a375b0aa3fa, and squashed.

[chrysn]

bors merge

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• Murdock
• static-tests
• tools-build-success