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

[Jerome Forissier]

On 11/20/24 17:07, Heinrich Schuchardt wrote:
> On 20.11.24 15:52, Adriano Córdova wrote:
>>
>>
>> El mar, 19 nov 2024 a las 7:14, Heinrich Schuchardt (<heinrich.schuchardt at canonical.com <mailto:heinrich.schuchardt at 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 at gmail.com
>>     <mailto:adrianox at 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
> 
> Understood.
> 
> @Jerome
> Should the function ip_to_string be() moved to net/net-common.c in future to implement the missing commands in lwIP?

That would certainly be a good place for it, yes.

Thanks,
-- 
Jerome

> 
> Best regards
> 
> Heinrich
> 
>>
>> 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,
>>
>