| Message ID | 20230823083721.1318097-2-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add EFI HTTP boot support | expand |
Hi Kojima-san, On Wed, Aug 23, 2023 at 05:37:19PM +0900, Masahisa Kojima wrote: > This adds the URI device path option for 'boot add' subcommand. > User can add the URI load option for downloading ISO image file > or EFI application through network(e.g. HTTP). > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > index 0be3af3e76..62f867df2a 100644 > --- a/cmd/efidebug.c > +++ b/cmd/efidebug.c > @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > argc -= 1; > argv += 1; > break; > + case 'u': > + { > + char *pos; > + int uridp_len; > + struct efi_device_path_uri *uridp; > + > + if (argc < 3 || lo.label) { > + r = CMD_RET_USAGE; > + goto out; > + } > + id = (int)hextoul(argv[1], &endp); > + if (*endp != '\0' || id > 0xffff) > + return CMD_RET_USAGE; > + > + efi_create_indexed_name(var_name16, sizeof(var_name16), > + "Boot", id); > + > + label = efi_convert_string(argv[2]); > + if (!label) > + return CMD_RET_FAILURE; > + lo.label = label; > + > + uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; > + fp_free = efi_alloc(uridp_len + sizeof(END)); > + uridp = (struct efi_device_path_uri *)fp_free; > + uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; > + uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI; > + uridp->dp.length = uridp_len; > + strcpy(uridp->uri, argv[3]); > + pos = (char *)uridp + uridp_len; > + memcpy(pos, &END, sizeof(END)); > + fp_size += uridp_len + sizeof(END); > + file_path = (struct efi_device_path *)uridp; > + argc -= 3; > + argv += 3; > + break; > + } > + > default: > r = CMD_RET_USAGE; > goto out; > @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] = > " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" > " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" > " (-b, -i for short form device path)\n" > + " -u <bootid> <label> <uri>\n" It might be a matter of personal preference, but since the current syntax of efidebug is already much complex, I don't want to add more options unless it's necessary. How about re-using "-B" option, say => efidebug -B 3 debian-netinst uri - https://foo.com/baa BTW, I think that <bootid> and <label> should have been moved out of "-b|B" when Ilias introduced this new syntax. -Takahiro Akashi > " -s '<optional data>'\n" > "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n" > " - delete UEFI BootXXXX variables\n" > -- > 2.34.1 >
On 8/23/23 10:37, Masahisa Kojima wrote: > This adds the URI device path option for 'boot add' subcommand. > User can add the URI load option for downloading ISO image file > or EFI application through network(e.g. HTTP). > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > index 0be3af3e76..62f867df2a 100644 > --- a/cmd/efidebug.c > +++ b/cmd/efidebug.c > @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > argc -= 1; > argv += 1; > break; > + case 'u': > + { > + char *pos; > + int uridp_len; > + struct efi_device_path_uri *uridp; > + > + if (argc < 3 || lo.label) { > + r = CMD_RET_USAGE; > + goto out; > + } > + id = (int)hextoul(argv[1], &endp); > + if (*endp != '\0' || id > 0xffff) > + return CMD_RET_USAGE; > + > + efi_create_indexed_name(var_name16, sizeof(var_name16), > + "Boot", id); > + > + label = efi_convert_string(argv[2]); > + if (!label) > + return CMD_RET_FAILURE; > + lo.label = label; > + > + uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; > + fp_free = efi_alloc(uridp_len + sizeof(END)); > + uridp = (struct efi_device_path_uri *)fp_free; > + uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; > + uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI; > + uridp->dp.length = uridp_len; > + strcpy(uridp->uri, argv[3]); This assumes that argv[3] is a valid URI. Would it be preferable to validate that the string is percent encoded, conforms to RFC 3986, and contains a supported scheme, an authority, and a path? As a user I would like related errors to be caught at entry and not at runtime. Best regards Heinrich > + pos = (char *)uridp + uridp_len; > + memcpy(pos, &END, sizeof(END)); > + fp_size += uridp_len + sizeof(END); > + file_path = (struct efi_device_path *)uridp; > + argc -= 3; > + argv += 3; > + break; > + } > + > default: > r = CMD_RET_USAGE; > goto out; > @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] = > " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" > " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" > " (-b, -i for short form device path)\n" > + " -u <bootid> <label> <uri>\n" > " -s '<optional data>'\n" > "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n" > " - delete UEFI BootXXXX variables\n"
On Thu, 24 Aug 2023 at 09:11, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Hi Kojima-san, > > On Wed, Aug 23, 2023 at 05:37:19PM +0900, Masahisa Kojima wrote: > > This adds the URI device path option for 'boot add' subcommand. > > User can add the URI load option for downloading ISO image file > > or EFI application through network(e.g. HTTP). > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > --- > > cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > > index 0be3af3e76..62f867df2a 100644 > > --- a/cmd/efidebug.c > > +++ b/cmd/efidebug.c > > @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > > argc -= 1; > > argv += 1; > > break; > > + case 'u': > > + { > > + char *pos; > > + int uridp_len; > > + struct efi_device_path_uri *uridp; > > + > > + if (argc < 3 || lo.label) { > > + r = CMD_RET_USAGE; > > + goto out; > > + } > > + id = (int)hextoul(argv[1], &endp); > > + if (*endp != '\0' || id > 0xffff) > > + return CMD_RET_USAGE; > > + > > + efi_create_indexed_name(var_name16, sizeof(var_name16), > > + "Boot", id); > > + > > + label = efi_convert_string(argv[2]); > > + if (!label) > > + return CMD_RET_FAILURE; > > + lo.label = label; > > + > > + uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; > > + fp_free = efi_alloc(uridp_len + sizeof(END)); > > + uridp = (struct efi_device_path_uri *)fp_free; > > + uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; > > + uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI; > > + uridp->dp.length = uridp_len; > > + strcpy(uridp->uri, argv[3]); > > + pos = (char *)uridp + uridp_len; > > + memcpy(pos, &END, sizeof(END)); > > + fp_size += uridp_len + sizeof(END); > > + file_path = (struct efi_device_path *)uridp; > > + argc -= 3; > > + argv += 3; > > + break; > > + } > > + > > default: > > r = CMD_RET_USAGE; > > goto out; > > @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] = > > " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" > > " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" > > " (-b, -i for short form device path)\n" > > + " -u <bootid> <label> <uri>\n" > > It might be a matter of personal preference, but > since the current syntax of efidebug is already much complex, > I don't want to add more options unless it's necessary. > How about re-using "-B" option, say > => efidebug -B 3 debian-netinst uri - https://foo.com/baa I understand your concern. OK, I will add uri in -b|B and also update documentation. Thanks, Masahisa Kojima > > BTW, I think that <bootid> and <label> should have been moved out of "-b|B" > when Ilias introduced this new syntax. > > -Takahiro Akashi > > > > " -s '<optional data>'\n" > > "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n" > > " - delete UEFI BootXXXX variables\n" > > -- > > 2.34.1 > >
On Thu, 24 Aug 2023 at 14:37, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 8/23/23 10:37, Masahisa Kojima wrote: > > This adds the URI device path option for 'boot add' subcommand. > > User can add the URI load option for downloading ISO image file > > or EFI application through network(e.g. HTTP). > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > --- > > cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > > index 0be3af3e76..62f867df2a 100644 > > --- a/cmd/efidebug.c > > +++ b/cmd/efidebug.c > > @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > > argc -= 1; > > argv += 1; > > break; > > + case 'u': > > + { > > + char *pos; > > + int uridp_len; > > + struct efi_device_path_uri *uridp; > > + > > + if (argc < 3 || lo.label) { > > + r = CMD_RET_USAGE; > > + goto out; > > + } > > + id = (int)hextoul(argv[1], &endp); > > + if (*endp != '\0' || id > 0xffff) > > + return CMD_RET_USAGE; > > + > > + efi_create_indexed_name(var_name16, sizeof(var_name16), > > + "Boot", id); > > + > > + label = efi_convert_string(argv[2]); > > + if (!label) > > + return CMD_RET_FAILURE; > > + lo.label = label; > > + > > + uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; > + fp_free = efi_alloc(uridp_len + sizeof(END)); > > + uridp = (struct efi_device_path_uri *)fp_free; > > + uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; > > + uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI; > > + uridp->dp.length = uridp_len; > > + strcpy(uridp->uri, argv[3]); > > This assumes that argv[3] is a valid URI. > > Would it be preferable to validate that the string is percent encoded, > conforms to RFC 3986, and contains a supported scheme, an authority, and > a path? > > As a user I would like related errors to be caught at entry and not at > runtime. OK, I agree. I will add input uri validation here. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > + pos = (char *)uridp + uridp_len; > > + memcpy(pos, &END, sizeof(END)); > > + fp_size += uridp_len + sizeof(END); > > + file_path = (struct efi_device_path *)uridp; > > + argc -= 3; > > + argv += 3; > > + break; > > + } > > + > > default: > > r = CMD_RET_USAGE; > > goto out; > > @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] = > > " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" > > " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" > > " (-b, -i for short form device path)\n" > > + " -u <bootid> <label> <uri>\n" > > " -s '<optional data>'\n" > > "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n" > > " - delete UEFI BootXXXX variables\n" >
On 8/25/23 07:57, Masahisa Kojima wrote: > On Thu, 24 Aug 2023 at 09:11, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: >> >> Hi Kojima-san, >> >> On Wed, Aug 23, 2023 at 05:37:19PM +0900, Masahisa Kojima wrote: >>> This adds the URI device path option for 'boot add' subcommand. >>> User can add the URI load option for downloading ISO image file >>> or EFI application through network(e.g. HTTP). >>> >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >>> --- >>> cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c >>> index 0be3af3e76..62f867df2a 100644 >>> --- a/cmd/efidebug.c >>> +++ b/cmd/efidebug.c >>> @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, >>> argc -= 1; >>> argv += 1; >>> break; >>> + case 'u': >>> + { >>> + char *pos; >>> + int uridp_len; >>> + struct efi_device_path_uri *uridp; >>> + >>> + if (argc < 3 || lo.label) { >>> + r = CMD_RET_USAGE; >>> + goto out; >>> + } >>> + id = (int)hextoul(argv[1], &endp); >>> + if (*endp != '\0' || id > 0xffff) >>> + return CMD_RET_USAGE; >>> + >>> + efi_create_indexed_name(var_name16, sizeof(var_name16), >>> + "Boot", id); >>> + >>> + label = efi_convert_string(argv[2]); >>> + if (!label) >>> + return CMD_RET_FAILURE; >>> + lo.label = label; >>> + >>> + uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; >>> + fp_free = efi_alloc(uridp_len + sizeof(END)); >>> + uridp = (struct efi_device_path_uri *)fp_free; >>> + uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; >>> + uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI; >>> + uridp->dp.length = uridp_len; >>> + strcpy(uridp->uri, argv[3]); >>> + pos = (char *)uridp + uridp_len; >>> + memcpy(pos, &END, sizeof(END)); >>> + fp_size += uridp_len + sizeof(END); >>> + file_path = (struct efi_device_path *)uridp; >>> + argc -= 3; >>> + argv += 3; >>> + break; >>> + } >>> + >>> default: >>> r = CMD_RET_USAGE; >>> goto out; >>> @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] = >>> " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" >>> " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" >>> " (-b, -i for short form device path)\n" >>> + " -u <bootid> <label> <uri>\n" >> >> It might be a matter of personal preference, but >> since the current syntax of efidebug is already much complex, >> I don't want to add more options unless it's necessary. >> How about re-using "-B" option, say >> => efidebug -B 3 debian-netinst uri - https://foo.com/baa You just replace one set of alternatives (-B -b -u) against another (mmc, nvme, uri, ...). I cannot see that this reduces complexity. > > I understand your concern. OK, I will add uri in -b|B and also update > documentation. It is unclear to me what the difference between -b and -B shall be for URIs. Would -B be with the device path of the network device (/VenHw()/MAC()/URI()) and -b without (/URI())? Best regards Heinrich > > Thanks, > Masahisa Kojima > >> >> BTW, I think that <bootid> and <label> should have been moved out of "-b|B" >> when Ilias introduced this new syntax. >> >> -Takahiro Akashi >> >> >>> " -s '<optional data>'\n" >>> "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n" >>> " - delete UEFI BootXXXX variables\n" >>> -- >>> 2.34.1 >>>
Hi Heinrich, On Sat, 26 Aug 2023 at 11:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 8/25/23 07:57, Masahisa Kojima wrote: > > On Thu, 24 Aug 2023 at 09:11, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > >> > >> Hi Kojima-san, > >> > >> On Wed, Aug 23, 2023 at 05:37:19PM +0900, Masahisa Kojima wrote: > >>> This adds the URI device path option for 'boot add' subcommand. > >>> User can add the URI load option for downloading ISO image file > >>> or EFI application through network(e.g. HTTP). > >>> > >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > >>> --- > >>> cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 39 insertions(+) > >>> > >>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c > >>> index 0be3af3e76..62f867df2a 100644 > >>> --- a/cmd/efidebug.c > >>> +++ b/cmd/efidebug.c > >>> @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > >>> argc -= 1; > >>> argv += 1; > >>> break; > >>> + case 'u': > >>> + { > >>> + char *pos; > >>> + int uridp_len; > >>> + struct efi_device_path_uri *uridp; > >>> + > >>> + if (argc < 3 || lo.label) { > >>> + r = CMD_RET_USAGE; > >>> + goto out; > >>> + } > >>> + id = (int)hextoul(argv[1], &endp); > >>> + if (*endp != '\0' || id > 0xffff) > >>> + return CMD_RET_USAGE; > >>> + > >>> + efi_create_indexed_name(var_name16, sizeof(var_name16), > >>> + "Boot", id); > >>> + > >>> + label = efi_convert_string(argv[2]); > >>> + if (!label) > >>> + return CMD_RET_FAILURE; > >>> + lo.label = label; > >>> + > >>> + uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; > >>> + fp_free = efi_alloc(uridp_len + sizeof(END)); > >>> + uridp = (struct efi_device_path_uri *)fp_free; > >>> + uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; > >>> + uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI; > >>> + uridp->dp.length = uridp_len; > >>> + strcpy(uridp->uri, argv[3]); > >>> + pos = (char *)uridp + uridp_len; > >>> + memcpy(pos, &END, sizeof(END)); > >>> + fp_size += uridp_len + sizeof(END); > >>> + file_path = (struct efi_device_path *)uridp; > >>> + argc -= 3; > >>> + argv += 3; > >>> + break; > >>> + } > >>> + > >>> default: > >>> r = CMD_RET_USAGE; > >>> goto out; > >>> @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] = > >>> " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" > >>> " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" > >>> " (-b, -i for short form device path)\n" > >>> + " -u <bootid> <label> <uri>\n" > >> > >> It might be a matter of personal preference, but > >> since the current syntax of efidebug is already much complex, > >> I don't want to add more options unless it's necessary. > >> How about re-using "-B" option, say > >> => efidebug -B 3 debian-netinst uri - https://foo.com/baa > > You just replace one set of alternatives (-B -b -u) against another > (mmc, nvme, uri, ...). I cannot see that this reduces complexity. > > > > > I understand your concern. OK, I will add uri in -b|B and also update > > documentation. > > It is unclear to me what the difference between -b and -B shall be for > URIs. Would -B be with the device path of the network device > (/VenHw()/MAC()/URI()) and -b without (/URI())? Differentiating -b and -B for URI is not mandatory, I think. To keep implementation simple, I would like to add '-u' option as the original patch does. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > > > Thanks, > > Masahisa Kojima > > > >> > >> BTW, I think that <bootid> and <label> should have been moved out of "-b|B" > >> when Ilias introduced this new syntax. > >> > >> -Takahiro Akashi > >> > >> > >>> " -s '<optional data>'\n" > >>> "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n" > >>> " - delete UEFI BootXXXX variables\n" > >>> -- > >>> 2.34.1 > >>> >
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 0be3af3e76..62f867df2a 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, argc -= 1; argv += 1; break; + case 'u': + { + char *pos; + int uridp_len; + struct efi_device_path_uri *uridp; + + if (argc < 3 || lo.label) { + r = CMD_RET_USAGE; + goto out; + } + id = (int)hextoul(argv[1], &endp); + if (*endp != '\0' || id > 0xffff) + return CMD_RET_USAGE; + + efi_create_indexed_name(var_name16, sizeof(var_name16), + "Boot", id); + + label = efi_convert_string(argv[2]); + if (!label) + return CMD_RET_FAILURE; + lo.label = label; + + uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; + fp_free = efi_alloc(uridp_len + sizeof(END)); + uridp = (struct efi_device_path_uri *)fp_free; + uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; + uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI; + uridp->dp.length = uridp_len; + strcpy(uridp->uri, argv[3]); + pos = (char *)uridp + uridp_len; + memcpy(pos, &END, sizeof(END)); + fp_size += uridp_len + sizeof(END); + file_path = (struct efi_device_path *)uridp; + argc -= 3; + argv += 3; + break; + } + default: r = CMD_RET_USAGE; goto out; @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] = " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" " (-b, -i for short form device path)\n" + " -u <bootid> <label> <uri>\n" " -s '<optional data>'\n" "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n" " - delete UEFI BootXXXX variables\n"
This adds the URI device path option for 'boot add' subcommand. User can add the URI load option for downloading ISO image file or EFI application through network(e.g. HTTP). Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)