Re: [PATCH v4 07/14] efi_loader: device_path: add support for HTTP device path

Adriano Córdova
2024-11-20T14:52:48Z

El mar, 19 nov 2024 a las 7:14, Heinrich Schuchardt (<
heinrich.schuchardt@canonical.com>) escribió:

> On 18.11.24 22:09, Adriano Cordova wrote:
> > Add efi_dp_from_http to form a device path from HTTP. The
> > device path is the concatenation of the device path returned
> > by efi_dp_from_ipv4 together with an URI node and an END node.
> >
> > Signed-off-by: Adriano Cordova <adrianox@gmail.com>
> > ---
> > Changes in v4:
> > - Reworked an if-else
> >
> > Changes in v3:
> > - Moved argument checks in efi_dp_from_http to the beginning of the
> function
> > include/efi_loader.h | 1 +
> > lib/efi_loader/efi_device_path.c | 52 ++++++++++++++++++++++++++++++++
> > 2 files changed, 53 insertions(+)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 612bc42816..96b204dfc3 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -872,6 +872,7 @@ struct efi_device_path *efi_dp_part_node(struct
> blk_desc *desc, int part);
> > struct efi_device_path *efi_dp_from_file(const struct efi_device_path
> *dp,
> > const char *path);
> > struct efi_device_path *efi_dp_from_eth(void);
> > +struct efi_device_path *efi_dp_from_http(const char *server);
> > struct efi_device_path *efi_dp_from_mem(uint32_t mem_type,
> > uint64_t start_address,
> > size_t size);
> > diff --git a/lib/efi_loader/efi_device_path.c
> b/lib/efi_loader/efi_device_path.c
> > index cdeea4791f..9ee03062ac 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -1012,6 +1012,58 @@ struct efi_device_path *efi_dp_from_ipv4(struct
> efi_ipv4_address *ip,
> > return dp2;
> > }
> >
>
> A function description is missing.
> Please,
>
> > +struct efi_device_path *efi_dp_from_http(const char *server)
> > +{
> > + struct efi_device_path *dp1, *dp2;
> > + struct efi_device_path_uri *uridp;
> > + efi_uintn_t uridp_len;
> > + char *pos;
> > + char tmp[128];
> > + struct efi_ipv4_address ip;
> > + struct efi_ipv4_address mask;
> > +
> > + if ((server && strlen("http://") + strlen(server) + 1 >
> sizeof(tmp)) ||
> > + (!server && IS_ENABLED(CONFIG_NET_LWIP)))
> > + return NULL;
> > +
> > + efi_net_get_addr(&ip, &mask, NULL);
> > +
> > + dp1 = efi_dp_from_ipv4(&ip, &mask, NULL);
>
> This seems to be the only usage of efi_dp_from_ipv4(). So we should make
> it static.
>
> dp1 is expected to be NULL if we are out of memory. This error needs to
> be handled.
>
> > +
> > + strcpy(tmp, "http://");
>
> @Ilias:
> Linaro sent a patch series for supporting https.
> Will that go in after this series?
>
> > +
> > + if (server) {
> > + memcpy(tmp + strlen("http://"), server, strlen(server) +
> 1);
>
> Please, use strcat() to simplify this line.
>
> > + }
> > +#if !IS_ENABLED(CONFIG_NET_LWIP)
>
> Please use 'else if' instead of '#if'.
>
> Best regards
>
> Heinrich
>

Hi Heinrich,
I can't use 'else if' as the symbol net_server_ip is only defined in legacy
net

Best,
Adriano

>
> > + else {
> > + ip_to_string(net_server_ip, tmp + strlen("http://"));
> > + }
> > +#endif
> > +
> > + uridp_len = sizeof(struct efi_device_path) + strlen(tmp) + 1;
> > + uridp = efi_alloc(uridp_len + sizeof(END));
> > + if (!uridp) {
> > + log_err("Out of memory\n");
> > + 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;
> > + debug("device path: setting uri device path to %s\n", tmp);
> > + memcpy(uridp->uri, tmp, strlen(tmp) + 1);
> > +
> > + pos = (char *)uridp + uridp_len;
> > + memcpy(pos, &END, sizeof(END));
> > +
> > + dp2 = efi_dp_concat(dp1, (const struct efi_device_path *)uridp, 0);
> > +
> > + efi_free_pool(uridp);
> > + efi_free_pool(dp1);
> > +
> > + return dp2;
> > +}
> > +
> > /* Construct a device-path for memory-mapped image */
> > struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
> > uint64_t start_address,
>
>