remove storage callbacks

[TychoVrahe]

Our storage implementation currently relies on callbacks to show UI progress.

After core-split to kernel and core-app, these callbacks became very complicated, as jumping from syscall back to applet is not straightforward.

Therefore, it would be beneficial to redesign the storage calls so that these callbacks would not be needed.

[matejcik]

what are our options here?

would it make sense to move the loader drawing to kernel so that the callback works without crossing the boundary?

another alternative comes to mind is, instead of a generic callback mechanism, install a single "progress has been made" event for the coreapp to pick up, and attach the callback in "userspace"?

[TychoVrahe]

cc @cepetr as he had some ideas

[cepetr]

I would like to eliminate any callbacks and use a different approach to address the problem. Unfortunately, it’s not simple to implement and requires extensive changes to the code. However, I believe it’s the most future-proof solution:

Here’s the idea:

typedef struct {
    // Progress expressed as a ratio: progress/total
    int progress;
    int total;
    // Result of the operation
    secbool result;
} storage_wipe_progress_t;

// Starts a wipe operation and returns immediately.
// Returns `secfalse` if the operation can't be started.
secbool storage_wipe_begin(void);

// Performs some work on the ongoing operation and returns.
// This function should be called in a loop until it returns `secfalse`.
// The caller should check the `result` field of the progress struct.
secbool storage_wipe_continue(storage_wipe_progress_t* progress);

// Aborts the wipe operation.
void storage_wipe_abort(void);

There are currently five functions that use callbacks and need to be modified:

storage_init();
storage_wipe();
storage_unlock();
storage_change_pin();
storage_change_wipecode();

Maybe we can make the solution more generic:

typedef int op_handle_t;

// Start various operations
secbool storage_wipe_begin(op_handle_t* handle);
secbool storage_unlock_begin(..., op_handle_t* handle);
secbool storage_change_pin(..., op_handle_t* handle);

// Performs some work on the specified operation and returns.
// This function should be called in a loop until it returns `secfalse`.
secbool storage_continue(op_handle_t handle, storage_progress_t* progress);

// Aborts the specified storage operation
void storage_abort(op_handle_t handle);

[matejcik]

I'd kind of like to avoid manually implementing continuations ;) one pretty strong argument against: coreapp should not have the capability to not continue an operation.
(sure, you can mess things up in a callback, but let's not make that any easier)

There are currently five functions that use callbacks and need to be modified:

there's also fatfs formatting, but that probably stayed in coreapp?

[cepetr]

coreapp should not have the capability to not continue an operation.

Why shouldn’t coreapp have the capability to not continue an operation? Without it, how could a user, for example, cancel a long operation? Maybe we don’t need it now, but in general, I find this quite useful.

there's also fatfs formatting, but that probably stayed in coreapp?

Yes, it’s now implemented in coreapp. However, we could (maybe should) change it in the future.