Hi David, On Wed, 13 May 2026 at 00:48, David Lechner <[email protected]> wrote: > > Split wget_do_request() into two functions to make error handling less > error-prone. > > After a successful call to net_lwip_new_netif(), net_lwip_remove_netif() > must always be called to leaks.
prevent leaks* > This was missed in the HTTPS section of > the code where we returned on error without cleaning up. I'd prefer making this a bit more explicit. net_lwip_remove_netif() was called in case of a failure afaict. The only place that wasn's called is when checking the cacert_auth_mode, right? Anyway, I think Jerome can fix them up during the PR Acked-by: Ilias Apalodimas <[email protected]> Thanks /Ilias > > Instead of adding more calls to net_lwip_remove_netif(), refactor the > code into two functions. The outer function handles managing the netif > lifecycle. The inner function no longer has to worry about cleaning up > before returning on error. > > To keep things simple, the `path` local variable is removed during the > refactoring. Instead, ctx.path is used directly everywhere. > > Fixes: 3c656c928bd7 ("net: lwip: add wget command") > Signed-off-by: David Lechner <[email protected]> > --- > net/lwip/wget.c | 88 > ++++++++++++++++++++++++++++++--------------------------- > 1 file changed, 47 insertions(+), 41 deletions(-) > > diff --git a/net/lwip/wget.c b/net/lwip/wget.c > index 502c0faebb2..f89a6580b41 100644 > --- a/net/lwip/wget.c > +++ b/net/lwip/wget.c > @@ -292,45 +292,17 @@ static err_t httpc_headers_done_cb(httpc_state_t > *connection, void *arg, struct > #if CONFIG_IS_ENABLED(WGET_CACERT) > #endif > > -int wget_do_request(ulong dst_addr, char *uri) > +static int wget_handle_request(struct wget_ctx *ctx, bool is_https, > + struct udevice *udev, struct netif *netif) > { > #if CONFIG_IS_ENABLED(WGET_HTTPS) > altcp_allocator_t tls_allocator; > #endif > httpc_connection_t conn; > httpc_state_t *state; > - struct udevice *udev; > - struct netif *netif; > - struct wget_ctx ctx; > - char *path; > - bool is_https; > - > - ctx.daddr = dst_addr; > - ctx.saved_daddr = dst_addr; > - ctx.done = NOT_DONE; > - ctx.size = 0; > - ctx.prevsize = 0; > - ctx.start_time = 0; > - ctx.content_len = 0; > - ctx.hash_count = 0; > - > - if (parse_url(uri, ctx.server_name, &ctx.port, &path, &is_https)) > - return CMD_RET_USAGE; > - > - if (net_lwip_eth_start() < 0) > - return CMD_RET_FAILURE; > - > - if (!wget_info) > - wget_info = &default_wget_info; > - > - udev = eth_get_dev(); > - > - netif = net_lwip_new_netif(udev); > - if (!netif) > - return -1; > > /* if URL with hostname init dns */ > - if (!ipaddr_aton(ctx.server_name, NULL) && net_lwip_dns_init()) > + if (!ipaddr_aton(ctx->server_name, NULL) && net_lwip_dns_init()) > return CMD_RET_FAILURE; > > memset(&conn, 0, sizeof(conn)); > @@ -374,11 +346,10 @@ int wget_do_request(ulong dst_addr, char *uri) > tls_allocator.alloc = &altcp_tls_alloc; > tls_allocator.arg = > altcp_tls_create_config_client(ca, ca_sz, > - ctx.server_name); > + ctx->server_name); > > if (!tls_allocator.arg) { > log_err("error: Cannot create a TLS connection\n"); > - net_lwip_remove_netif(netif); > return -1; > } > > @@ -388,24 +359,20 @@ int wget_do_request(ulong dst_addr, char *uri) > > conn.result_fn = httpc_result_cb; > conn.headers_done_fn = httpc_headers_done_cb; > - ctx.path = path; > - if (httpc_get_file_dns(ctx.server_name, ctx.port, path, &conn, > httpc_recv_cb, > - &ctx, &state)) { > - net_lwip_remove_netif(netif); > + if (httpc_get_file_dns(ctx->server_name, ctx->port, ctx->path, &conn, > + httpc_recv_cb, ctx, &state)) { > return CMD_RET_FAILURE; > } > > errno = 0; > > - while (!ctx.done) { > + while (!ctx->done) { > net_lwip_rx(udev, netif); > if (ctrlc()) > break; > } > > - net_lwip_remove_netif(netif); > - > - if (ctx.done == SUCCESS) > + if (ctx->done == SUCCESS) > return 0; > > if (errno == EPERM && !wget_info->silent) > @@ -414,6 +381,45 @@ int wget_do_request(ulong dst_addr, char *uri) > return -1; > } > > +int wget_do_request(ulong dst_addr, char *uri) > +{ > + struct udevice *udev; > + struct wget_ctx ctx; > + struct netif *netif; > + bool is_https; > + int ret; > + > + ctx.daddr = dst_addr; > + ctx.saved_daddr = dst_addr; > + ctx.done = NOT_DONE; > + ctx.size = 0; > + ctx.prevsize = 0; > + ctx.start_time = 0; > + ctx.content_len = 0; > + ctx.hash_count = 0; > + > + if (parse_url(uri, ctx.server_name, &ctx.port, &ctx.path, &is_https)) > + return CMD_RET_USAGE; > + > + if (net_lwip_eth_start() < 0) > + return CMD_RET_FAILURE; > + > + if (!wget_info) > + wget_info = &default_wget_info; > + > + udev = eth_get_dev(); > + > + netif = net_lwip_new_netif(udev); > + if (!netif) > + return -1; > + > + ret = wget_handle_request(&ctx, is_https, udev, netif); > + > + net_lwip_remove_netif(netif); > + > + return ret; > +} > + > /** > * wget_validate_uri() - validate the uri for wget > * > > -- > 2.43.0 >
