diff options
author | Joel Kitching <kitching@google.com> | 2020-05-05 10:34:11 +0800 |
---|---|---|
committer | Commit Bot <commit-bot@chromium.org> | 2020-05-07 09:58:54 +0000 |
commit | 54ca8c3a2516bde0097d439bd6a85ddb1ce320fa (patch) | |
tree | c2b19ae923407dffcd1662f5e77c0ff8700bb2bf | |
parent | 8c03950f59c86df17eca8dfe2a8bdc7ea3e075c7 (diff) | |
download | vboot-54ca8c3a2516bde0097d439bd6a85ddb1ce320fa.tar.gz |
vboot/ui: remove validate_selection function
Given that we are sending the full vb2_ui_context into
UI-related functions, it's impossible to fully validate that
called functions don't modify UI state in unexpected ways.
Assume UI-related functions are mutating vb2_ui_context data
correctly. Screen init functions (see CL:2168072) will be
used to set selected_item and disabled_mask before displaying
a screen for the first time.
change_screen() is also changed to return a vb2_error_t value
to be more consistent with action functions.
BUG=b:146399181
TEST=make clean && make runtests
BRANCH=none
Change-Id: Icda68f95a835b9143b8dd085d8dbdb7bced04775
Signed-off-by: Joel Kitching <kitching@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2182084
Tested-by: Joel Kitching <kitching@chromium.org>
Commit-Queue: Yu-Ping Wu <yupingso@chromium.org>
Reviewed-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>
-rw-r--r-- | firmware/2lib/2ui.c | 52 | ||||
-rw-r--r-- | firmware/2lib/include/2ui_private.h | 3 | ||||
-rw-r--r-- | tests/vb2_ui_utility_tests.c | 63 |
3 files changed, 25 insertions, 93 deletions
diff --git a/firmware/2lib/2ui.c b/firmware/2lib/2ui.c index 8eea9afb..3c54ecca 100644 --- a/firmware/2lib/2ui.c +++ b/firmware/2lib/2ui.c @@ -138,12 +138,11 @@ vb2_error_t menu_select_action(struct vb2_ui_context *ui) if (menu_item->target) { VB2_DEBUG("Changing to target screen %#x for menu item <%s>\n", menu_item->target, menu_item->text); - change_screen(ui, menu_item->target); - } else { - VB2_DEBUG("No target set for menu item <%s>\n", - menu_item->text); + return change_screen(ui, menu_item->target); } + VB2_DEBUG("No target screen for menu item <%s>\n", menu_item->text); + return VB2_REQUEST_UI_CONTINUE; } @@ -152,8 +151,8 @@ vb2_error_t menu_select_action(struct vb2_ui_context *ui) */ vb2_error_t menu_back_action(struct vb2_ui_context *ui) { - change_screen(ui, ui->root_screen->id); - return VB2_REQUEST_UI_CONTINUE; + /* TODO(kitching): Return to previous screen instead of root screen. */ + return change_screen(ui, ui->root_screen->id); } /*****************************************************************************/ @@ -181,33 +180,19 @@ vb2_error_t (*input_action_lookup(int key))(struct vb2_ui_context *ui) /*****************************************************************************/ /* Core UI functions */ -void change_screen(struct vb2_ui_context *ui, enum vb2_screen id) +vb2_error_t change_screen(struct vb2_ui_context *ui, enum vb2_screen id) { const struct vb2_screen_info *new_screen_info = vb2_get_screen_info(id); + if (new_screen_info == NULL) { VB2_DEBUG("ERROR: Screen entry %#x not found; ignoring\n", id); - } else { - memset(&ui->state, 0, sizeof(ui->state)); - ui->state.screen = new_screen_info; + return VB2_REQUEST_UI_CONTINUE; } -} -void validate_selection(struct vb2_screen_state *state) -{ - if ((state->selected_item == 0 && state->screen->num_items == 0) || - (state->selected_item < state->screen->num_items && - !((1 << state->selected_item) & state->disabled_item_mask))) - return; - - /* Selection invalid; select the first available non-disabled item. */ - state->selected_item = 0; - while (((1 << state->selected_item) & state->disabled_item_mask) && - state->selected_item < state->screen->num_items) - state->selected_item++; - - /* No non-disabled items available; just choose 0. */ - if (state->selected_item >= state->screen->num_items) - state->selected_item = 0; + memset(&ui->state, 0, sizeof(ui->state)); + ui->state.screen = new_screen_info; + + return VB2_REQUEST_UI_CONTINUE; } vb2_error_t ui_loop(struct vb2_context *ctx, enum vb2_screen root_screen_id, @@ -225,7 +210,9 @@ vb2_error_t ui_loop(struct vb2_context *ctx, enum vb2_screen root_screen_id, ui.root_screen = vb2_get_screen_info(root_screen_id); if (ui.root_screen == NULL) VB2_DIE("Root screen not found.\n"); - change_screen(&ui, ui.root_screen->id); + rv = change_screen(&ui, ui.root_screen->id); + if (rv != VB2_REQUEST_UI_CONTINUE) + return rv; memset(&prev_state, 0, sizeof(prev_state)); while (1) { @@ -259,7 +246,6 @@ vb2_error_t ui_loop(struct vb2_context *ctx, enum vb2_screen root_screen_id, ui.key = 0; if (rv != VB2_REQUEST_UI_CONTINUE) return rv; - validate_selection(&ui.state); } else if (key) { VB2_DEBUG("Pressed key %#x, trusted? %d\n", key, !!(key_flags & VB_KEY_FLAG_TRUSTED_KEYBOARD)); @@ -268,7 +254,6 @@ vb2_error_t ui_loop(struct vb2_context *ctx, enum vb2_screen root_screen_id, /* Run global action function if available. */ if (global_action) { rv = global_action(&ui); - validate_selection(&ui.state); if (rv != VB2_REQUEST_UI_CONTINUE) return rv; } @@ -336,10 +321,9 @@ vb2_error_t try_recovery_action(struct vb2_ui_context *ui) invalid_disk = rv != VB2_ERROR_LK_NO_DISK_FOUND; if (invalid_disk_last != invalid_disk) { invalid_disk_last = invalid_disk; - if (invalid_disk) - change_screen(ui, VB2_SCREEN_RECOVERY_INVALID); - else - change_screen(ui, VB2_SCREEN_RECOVERY_SELECT); + return change_screen(ui, invalid_disk ? + VB2_SCREEN_RECOVERY_INVALID : + VB2_SCREEN_RECOVERY_SELECT); } return VB2_REQUEST_UI_CONTINUE; diff --git a/firmware/2lib/include/2ui_private.h b/firmware/2lib/include/2ui_private.h index 40ee6b51..00534492 100644 --- a/firmware/2lib/include/2ui_private.h +++ b/firmware/2lib/include/2ui_private.h @@ -31,8 +31,7 @@ vb2_error_t menu_select_action(struct vb2_ui_context *ui); vb2_error_t menu_back_action(struct vb2_ui_context *ui); vb2_error_t (*input_action_lookup(int key))(struct vb2_ui_context *ui); -void change_screen(struct vb2_ui_context *ui, enum vb2_screen id); -void validate_selection(struct vb2_screen_state *state); +vb2_error_t change_screen(struct vb2_ui_context *ui, enum vb2_screen id); vb2_error_t ui_loop(struct vb2_context *ctx, enum vb2_screen root_screen_id, vb2_error_t (*global_action)(struct vb2_ui_context *ui)); diff --git a/tests/vb2_ui_utility_tests.c b/tests/vb2_ui_utility_tests.c index da7aaf72..e62f6c05 100644 --- a/tests/vb2_ui_utility_tests.c +++ b/tests/vb2_ui_utility_tests.c @@ -614,72 +614,22 @@ static void change_screen_tests(void) mock_state->screen = &mock_screen_menu; mock_state->selected_item = 2; mock_state->disabled_item_mask = 0x10; - VB2_DEBUG("change_screen will clear screen state\n"); - change_screen(&mock_ui_context, MOCK_SCREEN_BASE); + TEST_EQ(change_screen(&mock_ui_context, MOCK_SCREEN_BASE), + VB2_REQUEST_UI_CONTINUE, + "change_screen will clear screen state"); screen_state_eq(mock_state, MOCK_SCREEN_BASE, 0, 0); /* Change to screen which does not exist */ reset_common_data(); mock_state->screen = &mock_screen_menu; - VB2_DEBUG("change to screen which does not exist\n"); - change_screen(&mock_ui_context, MOCK_NO_SCREEN); + TEST_EQ(change_screen(&mock_ui_context, MOCK_NO_SCREEN), + VB2_REQUEST_UI_CONTINUE, + "change to screen which does not exist"); screen_state_eq(mock_state, MOCK_SCREEN_MENU, MOCK_IGNORE, MOCK_IGNORE); VB2_DEBUG("...done.\n"); } -static void validate_selection_tests(void) -{ - VB2_DEBUG("Testing validate_selection..."); - - /* No item */ - reset_common_data(); - mock_state->screen = &mock_screen_base; - mock_state->selected_item = 2; - mock_state->disabled_item_mask = 0x10; - VB2_DEBUG("no item (fix selected_item)\n"); - validate_selection(mock_state); - screen_state_eq(mock_state, MOCK_SCREEN_BASE, 0, MOCK_IGNORE); - - /* Valid selected_item */ - reset_common_data(); - mock_state->screen = &mock_screen_menu; - mock_state->selected_item = 2; - mock_state->disabled_item_mask = 0x13; /* 0b10011 */ - VB2_DEBUG("valid selected_item\n"); - validate_selection(mock_state); - screen_state_eq(mock_state, MOCK_SCREEN_MENU, 2, MOCK_IGNORE); - - /* selected_item too large */ - reset_common_data(); - mock_state->screen = &mock_screen_menu; - mock_state->selected_item = 5; - mock_state->disabled_item_mask = 0x15; /* 0b10101 */ - VB2_DEBUG("selected_item too large\n"); - validate_selection(mock_state); - screen_state_eq(mock_state, MOCK_SCREEN_MENU, 1, MOCK_IGNORE); - - /* Select a disabled item */ - reset_common_data(); - mock_state->screen = &mock_screen_menu; - mock_state->selected_item = 4; - mock_state->disabled_item_mask = 0x17; /* 0b10111 */ - VB2_DEBUG("select a disabled item\n"); - validate_selection(mock_state); - screen_state_eq(mock_state, MOCK_SCREEN_MENU, 3, MOCK_IGNORE); - - /* No available item */ - reset_common_data(); - mock_state->screen = &mock_screen_menu; - mock_state->selected_item = 2; - mock_state->disabled_item_mask = 0x1f; /* 0b11111 */ - VB2_DEBUG("no available item\n"); - validate_selection(mock_state); - screen_state_eq(mock_state, MOCK_SCREEN_MENU, 0, MOCK_IGNORE); - - VB2_DEBUG("...done.\n"); -} - static void ui_loop_tests(void) { VB2_DEBUG("Testing ui_loop...\n"); @@ -781,7 +731,6 @@ int main(void) shutdown_required_tests(); menu_action_tests(); change_screen_tests(); - validate_selection_tests(); ui_loop_tests(); return gTestSuccess ? 0 : 255; |