On 26.06.25 11:28, Sughosh Ganu wrote:
On Thu, 26 Jun 2025 at 13:52, Heinrich Schuchardt <[email protected]> wrote:On 24.06.25 14:07, Sughosh Ganu wrote:The eficonfig command provides a menu based interface for maintenance of the EFI boot options. Add support for adding a URI based boot option. This boot option can then be used for HTTP boot. Signed-off-by: Sughosh Ganu <[email protected]> --- Changes since V1: * Move the option for entering the URI under the boot file. * All corresponding changes needed for the above change. cmd/eficonfig.c | 113 ++++++++++++++++++++++++++++++++++++++++--- include/efi_config.h | 4 ++ 2 files changed, 109 insertions(+), 8 deletions(-) diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 6e14d34a6bd..917d79b8023 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -35,6 +35,7 @@ static int avail_row; #define EFICONFIG_DESCRIPTION_MAX 32 #define EFICONFIG_OPTIONAL_DATA_MAX 64 +#define EFICONFIG_URI_MAX 512I am not aware of as standard setting a limit on URI sizes. https://www.rfc-editor.org/rfc/rfc9110#section-4.1-5 has this recommendation: It is RECOMMENDED that all senders and recipients support, at a minimum, URIs with lengths of 8000 octets in protocol elements. We should determine the length of the URI when the user makes his input.I had checked the corresponding RFC to check if there was a mention of size limit on the URI, and there is none. There is a mention about some applications using 2K as a limit for the URI length. But in U-Boot's context, I think that 512 bytes is a good enough limit, given that we are using the URI primarily for specifying distro installation image's path. Also, the handle_user_input() function in the eficonfig tool expects a size limit to be provided for all the user input, and I think this is a sane thing to do rather than allowing the user to input random strings. Do you see a realistic use-case where a URI of more than 512 characters might be expected for a boot option?
I do not control how people are setting up their image servers. E.g. if you create images on the fly and pass GET parameters URI's can become long. That is why I refer to the RFC which indicates bounds. But I agree that in most cases 512 will be enough.
You are using efi_console_get_u16_string() which is not usable to edit strings that are longer than a screen line properly. If I enter 512 times the character "a" and the backspace 512 times, I get:
** Edit URI ** enter HTTP Boot URI: a a a aI did not expect "Press ENTER to complete, ESC to quit" to be overwritten and I did not expect characters at the start of the line not being deleted when backspacing.
#define EFICONFIG_MENU_HEADER_ROW_NUM 3 #define EFICONFIG_MENU_DESC_ROW_NUM 5 @@ -538,6 +539,31 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_ return dp; } +static struct efi_device_path *eficonfig_create_uri_device_path(u16 *uri_str) +{ + char *pos, *p; + u32 len = 0; + efi_uintn_t uridp_len; + struct efi_device_path_uri *uridp; + + len = utf16_utf8_strlen(uri_str); + + uridp_len = sizeof(struct efi_device_path) + len + 1; + uridp = efi_alloc(uridp_len + sizeof(EFI_DP_END)); + if (!uridp) + return NULL; + + uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; + uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI; + uridp->dp.length = uridp_len; + p = (char *)&uridp->uri; + utf16_utf8_strcpy(&p, uri_str); + pos = (char *)uridp + uridp_len; + memcpy(pos, &EFI_DP_END, sizeof(EFI_DP_END)); + + return &uridp->dp; +} + /** * eficonfig_file_selected() - handler of file selection * @@ -983,6 +1009,22 @@ static efi_status_t eficonfig_boot_add_optional_data(void *data) " enter optional data:"); } +/** + * eficonfig_boot_add_uri() - handle user input for HTTP Boot URI + * + * @data: pointer to the internal boot option structure + * Return: status code + */ +static efi_status_t eficonfig_boot_add_uri(void *data) +{ + struct eficonfig_select_file_info *file_info = data; + + return handle_user_input(file_info->uri, EFICONFIG_URI_MAX, 24, + "\n ** Edit URI **\n" + "\n" + " enter HTTP Boot URI:"); +} + /** * eficonfig_boot_edit_save() - handler to save the boot option * @@ -998,7 +1040,8 @@ static efi_status_t eficonfig_boot_edit_save(void *data) bo->edit_completed = false; return EFI_NOT_READY; } - if (u16_strlen(bo->file_info.current_path) == 0) { + if (u16_strlen(bo->file_info.current_path) == 0 && + u16_strlen(bo->file_info.uri) == 0) { eficonfig_print_msg("File is not selected!"); bo->edit_completed = false; return EFI_NOT_READY; @@ -1027,6 +1070,13 @@ efi_status_t eficonfig_process_clear_file_selection(void *data) return EFI_ABORTED; } +static struct eficonfig_item select_boot_file_menu_items[] = { + {"Select File", eficonfig_process_select_file}, + {"Enter URI", eficonfig_boot_add_uri}, + {"Clear", eficonfig_process_clear_file_selection}, + {"Quit", eficonfig_process_quit}, +}; + static struct eficonfig_item select_file_menu_items[] = { {"Select File", eficonfig_process_select_file}, {"Clear", eficonfig_process_clear_file_selection}, @@ -1042,20 +1092,35 @@ static struct eficonfig_item select_file_menu_items[] = { efi_status_t eficonfig_process_show_file_option(void *data) { efi_status_t ret; + unsigned int menu_count; struct efimenu *efi_menu; + struct eficonfig_item *menu_items; + struct eficonfig_select_file_info *file_info = data; + + menu_items = file_info->boot_file_option ? select_boot_file_menu_items : + select_file_menu_items; + + menu_count = file_info->boot_file_option ? + ARRAY_SIZE(select_boot_file_menu_items) : + ARRAY_SIZE(select_file_menu_items); - select_file_menu_items[0].data = data; - select_file_menu_items[1].data = data; - efi_menu = eficonfig_create_fixed_menu(select_file_menu_items, - ARRAY_SIZE(select_file_menu_items)); + menu_items[0].data = data; + menu_items[1].data = data; + menu_items[2].data = data; + + efi_menu = eficonfig_create_fixed_menu(menu_items, menu_count); if (!efi_menu) return EFI_OUT_OF_RESOURCES; - ret = eficonfig_process_common(efi_menu, " ** Update File **", + ret = eficonfig_process_common(efi_menu, + file_info->boot_file_option ? + " ** Update File/URI **" : + " ** Update File **", eficonfig_menu_desc, eficonfig_display_statusline, eficonfig_print_entry, eficonfig_choice_entry); + if (ret != EFI_SUCCESS) /* User selects "Clear" or "Quit" */ ret = EFI_NOT_READY; @@ -1268,6 +1333,7 @@ static efi_status_t prepare_file_selection_entry(struct efimenu *efi_menu, char u16_strlcat(file_name, file_info->current_path, len); ret = create_boot_option_entry(efi_menu, title, file_name, eficonfig_process_show_file_option, file_info); + out: free(devname); free(file_name); @@ -1340,6 +1406,16 @@ out: return ret; }Please, document all functions according to https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentationSo I have had this comment from you on my other patches as well. I am not adding any API in my patch. What exactly is the rule for adding function documentation? I can understand if some static function happens to be doing a lot of work, and then it would be easier to understand its functionality through comments. But I don't think any function being added in this patch qualifies for such documentation being needed.
Developers succeeding you will want to know what each function is doing. We should document non-API functions as well.
+static void fill_dp_uri(struct efi_device_path *dp, u16 **uri_str) +{ + u16 *p = *uri_str; + struct efi_device_path_uri *uridp; + + uridp = (struct efi_device_path_uri *)dp; + + utf8_utf16_strcpy(&p, uridp->uri); +} + /** * fill_file_info() - fill the file info from efi_device_path structure * @@ -1392,10 +1468,13 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo size_t len; efi_status_t ret; char *tmp = NULL, *p; + u16 *current_path = NULL; struct efi_load_option lo = {0}; efi_uintn_t dp_size; struct efi_device_path *dp = NULL; efi_uintn_t size = load_option_size; + struct efi_device_path *dp_volume = NULL; + struct efi_device_path *uri_dp = NULL; struct efi_device_path *device_dp = NULL; struct efi_device_path *initrd_dp = NULL; struct efi_device_path *fdt_dp = NULL; @@ -1464,6 +1543,14 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; } + bo->file_info.uri = calloc(1, EFICONFIG_URI_MAX * sizeof(u16));Where is the matching free()?There is one being added below.+ if (!bo->file_info.uri) { + ret = EFI_OUT_OF_RESOURCES; + goto out; + } + + bo->file_info.boot_file_option = true; + /* copy the preset value */ if (load_option) { ret = efi_deserialize_load_option(&lo, load_option, &size); @@ -1481,7 +1568,10 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo u16_strcpy(bo->description, lo.label); /* EFI image file path is a first instance */ - if (lo.file_path) + if (lo.file_path && EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, + MSG_URI)) + fill_dp_uri(lo.file_path, &bo->file_info.uri); + else fill_file_info(lo.file_path, &bo->file_info, device_dp); /* Initrd file path (optional) is placed at second instance. */ @@ -1512,6 +1602,9 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; } + if (utf16_utf8_strlen(bo->file_info.uri)) + uri_dp = eficonfig_create_uri_device_path(bo->file_info.uri); + if (bo->initrd_info.dp_volume) { dp = eficonfig_create_device_path(bo->initrd_info.dp_volume, bo->initrd_info.current_path); @@ -1536,7 +1629,10 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo efi_free_pool(dp); } - dp = eficonfig_create_device_path(bo->file_info.dp_volume, bo->file_info.current_path); + dp_volume = bo->file_info.dp_volume; + current_path = bo->file_info.current_path; + dp = uri_dp ? + uri_dp : eficonfig_create_device_path(dp_volume, current_path); if (!dp) { ret = EFI_OUT_OF_RESOURCES; goto out; @@ -1560,6 +1656,7 @@ out: free(tmp); free(bo->optional_data); free(bo->description); + free(bo->file_info.uri);This is the matching free.
ok Best regards Heinrich
-sughoshfree(bo->file_info.current_path); free(bo->initrd_info.current_path); free(bo->fdt_info.current_path); diff --git a/include/efi_config.h b/include/efi_config.h index d7c1601137e..4438933db6a 100644 --- a/include/efi_config.h +++ b/include/efi_config.h @@ -82,15 +82,19 @@ struct eficonfig_item { * @current_volume: pointer to the efi_simple_file_system_protocol * @dp_volume: pointer to device path of the selected device * @current_path: pointer to the selected file path string + * @uri: URI for HTTP Boot * @filepath_list: list_head structure for file path list * @file_selectred: flag indicates file selecting status + * @boot_file_option: flag indicating if option is for boot file */ struct eficonfig_select_file_info { struct efi_simple_file_system_protocol *current_volume; struct efi_device_path *dp_volume; u16 *current_path; + u16 *uri; struct list_head filepath_list; bool file_selected; + bool boot_file_option; }; void eficonfig_print_msg(char *msg);
