Hi David, On Mon, 11 May 2026 at 15:48, David Lechner <[email protected]> wrote: > > On 5/11/26 1:08 PM, Simon Glass wrote: > > Hi David, > > > > On 2026-05-09T17:36:26, David Lechner <[email protected]> wrote: > >> net: lwip: introduce net_lwip_eth_stop() function > >> > >> Add a introduce net_lwip_eth_stop() function and use that to stop the > >> network interface after each command that uses the network. > >> > >> This makes the behavior the same as the legacy net code and avoids > >> potential issues with the network interface being left in an active > >> state after a command finishes. > >> > >> Signed-off-by: David Lechner <[email protected]> > >> > >> cmd/lwip/ping.c | 10 ++++++++-- > >> cmd/lwip/sntp.c | 10 ++++++++-- > >> include/net-lwip.h | 1 + > >> net/lwip/dhcp.c | 15 +++++++++++---- > >> net/lwip/dns.c | 6 +++++- > >> net/lwip/net-lwip.c | 6 +++++- > >> net/lwip/nfs.c | 4 ++++ > >> net/lwip/tftp.c | 4 ++++ > >> net/lwip/wget.c | 34 ++++++++++++++++++++++------------ > >> 9 files changed, 68 insertions(+), 22 deletions(-) > > > >> diff --git a/net/lwip/wget.c b/net/lwip/wget.c > >> @@ -302,6 +302,7 @@ int wget_do_request(ulong dst_addr, char *uri) > >> struct udevice *udev; > >> struct netif *netif; > >> struct wget_ctx ctx; > >> + int ret = CMD_RET_USAGE; > >> char *path; > >> bool is_https; > > > > Initialising ret to CMD_RET_USAGE changes error paths: > > > > netif = net_lwip_new_netif(udev); > > if (!netif) > > goto out_stop; /* ret is still CMD_RET_USAGE */ > > > > if (!tls_allocator.arg) { > > log_err(...); > > goto out_remove_netif; /* ret is still CMD_RET_USAGE */ > > } > > > > The fall-through after 'Certificate verification failed' also lands on > > out_remove_netif with ret unchanged. These paths previously returned > > -1; now they return CMD_RET_USAGE (1), so the command framework prints > > usage for a network/TLS failure. > > The code I am looking at says: > > enum command_ret_t { > CMD_RET_SUCCESS, /* 0 = Success */ > CMD_RET_FAILURE, /* 1 = Failure */ > CMD_RET_USAGE = -1, /* Failure, please report 'usage' error */ > }; > > So I don't think these changed. CMD_RET_USAGE is -1. > > Happy to init it -1 though as you suggest if the name doesn't make > sense here. That is might be why it was -1 before instead of using > the macro anyway.
Ah yes I missed that this is a pre-existing bug. We should return usage only when parsing is wrong, not when something fails. So what you have here is fine and actually makes the bug more obvious in the code, which is good. Reviewed-by: Simon Glass <[email protected]> > > > > > wget_do_request() isn't a command handler either, so CMD_RET_USAGE is > > doubly out of place. Please initialise ret to -1 and set ret = > > CMD_RET_FAILURE explicitly only in the paths that previously returned > > that. > > > > It might make sense to split this up function (perhaps in a prior > > patch) to try to make the error handling easier? > > > Sure. Is there a common naming pattern for that in U-Boot. I would > call the inner function __wget_do_request(). But I know opinions on > such things can vary widely. :-) If you do take this on, I would say better to rename it, e.g. handle_request() - strictly speaking we are supposed to avoid leading underscores, since they indicate symbols used by the compiler/toolchain, particularly the double underscore. In practice you will see use of a leading underscore in the code. Separately, I am taking a look at more use of getopt(), which would make parsing more regular, but unfortunately adds code size. Regards, Simon
