On 5/12/26 9:36 AM, Jerome Forissier wrote: > > > On 12/05/2026 13:41, Simon Glass wrote: >> 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. > > +1 > >> So what you have here is fine and actually makes the bug more obvious >> in the code, which is good. > > Yeah :) > >> Reviewed-by: Simon Glass <[email protected]> > > Reviewed-by: Jerome Forissier <[email protected]> > > Thanks,
I have prepared a revision that splits this into two functions so the -1 is left untouched. If I am understanding correctly, it sounds like I should add another patch to make the return values of wget_do_request() consistent. The docs for this function says: * Return: zero on success, negative if failed And there is one existing caller checking err < 0 (efi_bootmgr.c). So I plan to replace all `return CMD_RET_FAILURE` with `return -1`. Unless we should return errno codes?
