| Message ID | 20240819103616.2260006-4-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netconsole: Populate dynamic entry even if netpoll fails | expand |
On Mon, 19 Aug 2024 03:36:13 -0700 Breno Leitao wrote: > - if (err) > - goto fail; > + if (!err) { > + nt->enabled = true; > + } else { > + pr_err("Not enabling netconsole. Netpoll setup failed\n"); > + if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC)) > + /* only fail if dynamic reconfiguration is set, > + * otherwise, keep the target in the list, but disabled. > + */ > + goto fail; > + } This will be better written as: if (err) { /* handle err */ } nt->enabled = true; As for the message would it be more helpful to indicate target will be disabled? Move the print after the check for dynamic and say "Netpoll setup failed, netconsole target will be disabled" ?
Hello Jakub, On Tue, Aug 20, 2024 at 04:27:25PM -0700, Jakub Kicinski wrote: > On Mon, 19 Aug 2024 03:36:13 -0700 Breno Leitao wrote: > > - if (err) > > - goto fail; > > + if (!err) { > > + nt->enabled = true; > > + } else { > > + pr_err("Not enabling netconsole. Netpoll setup failed\n"); > > + if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC)) > > + /* only fail if dynamic reconfiguration is set, > > + * otherwise, keep the target in the list, but disabled. > > + */ > > + goto fail; > > + } > > This will be better written as: > > if (err) { > /* handle err */ > } > > nt->enabled = true; I tried to do something like this, but, I was not able to come up with something like this. There are three cases we need to check here: netpoll_setup returned 0: nt->enabled = true (1) netpoll_setup returned !=0 if IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC) continue with nt->enabled disabeld (2) if IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC) is disabld: goto fail. (3) The cases are: 1) Everything is fine 2) netpoll failed, but we want to keep configfs enabled 3) netpoll failed and we don't care about configfs. Another way to write this is: err = netpoll_setup(&nt->np); if (err) { pr_err("Not enabling netconsole. Netpoll setup failed\n"); if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC)) goto fail } else { nt->enabled = true; } is it better? Or, Is there a even better way to write this? > As for the message would it be more helpful to indicate target will be > disabled? Move the print after the check for dynamic and say "Netpoll > setup failed, netconsole target will be disabled" ? In both cases the target will be disabled, right? In one case, it will populate the cmdline0 configfs (if CONFIG_NETCONSOLE_DYNAMIC is set), otherwise it will fail completely. Either way, netconsole will be disabled. Let me know if I am missing something here. Thanks for the review, --breno
On Wed, 21 Aug 2024 01:21:58 -0700 Breno Leitao wrote: > Another way to write this is: > > err = netpoll_setup(&nt->np); > if (err) { > pr_err("Not enabling netconsole. Netpoll setup failed\n"); > if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC)) > goto fail > } else { > nt->enabled = true; > } > > is it better? Or, Is there a even better way to write this? Yes, I think this is better! Or at least I wouldn't have made the same mistake reading it if it was written this way :) > > As for the message would it be more helpful to indicate target will be > > disabled? Move the print after the check for dynamic and say "Netpoll > > setup failed, netconsole target will be disabled" ? > > In both cases the target will be disabled, right? In one case, it will > populate the cmdline0 configfs (if CONFIG_NETCONSOLE_DYNAMIC is set), > otherwise it will fail completely. Either way, netconsole will be > disabled. No strong feelings. I was trying to highlight that it's a single target that ends up being disabled "netconsole disabled" sounds like the whole netconsole module is completely out of commission.
Hello Jakub, On Wed, Aug 21, 2024 at 03:49:26PM -0700, Jakub Kicinski wrote: > On Wed, 21 Aug 2024 01:21:58 -0700 Breno Leitao wrote: > > Another way to write this is: > > > > err = netpoll_setup(&nt->np); > > if (err) { > > pr_err("Not enabling netconsole. Netpoll setup failed\n"); > > if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC)) > > goto fail > > } else { > > nt->enabled = true; > > } > > > > is it better? Or, Is there a even better way to write this? > > Yes, I think this is better! Or at least I wouldn't have made the same > mistake reading it if it was written this way :) > > > > As for the message would it be more helpful to indicate target will be > > > disabled? Move the print after the check for dynamic and say "Netpoll > > > setup failed, netconsole target will be disabled" ? > > > > In both cases the target will be disabled, right? In one case, it will > > populate the cmdline0 configfs (if CONFIG_NETCONSOLE_DYNAMIC is set), > > otherwise it will fail completely. Either way, netconsole will be > > disabled. > > No strong feelings. I was trying to highlight that it's a single target > that ends up being disabled "netconsole disabled" sounds like the whole > netconsole module is completely out of commission. That is fair, let me print the cmdline number, so, we can see something as: netpoll: netconsole: local port 6666 netpoll: netconsole: local IPv6 address 2401:db00:3120:21a9:face:0:270:0 netpoll: netconsole: interface 'ethX' netpoll: netconsole: remote port 1514 netpoll: netconsole: remote IPv6 address 2803:6080:a89c:a670::1 netpoll: netconsole: remote ethernet address 02:90:fb:66:aa:e5 netpoll: netconsole: ethX doesn't exist, aborting netconsole: Not enabling netconsole for cmdline0. Netpoll setup failed
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 9b5f605fe87a..82e178b34e4b 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -1260,11 +1260,18 @@ static struct netconsole_target *alloc_param_target(char *target_config, goto fail; err = netpoll_setup(&nt->np); - if (err) - goto fail; + if (!err) { + nt->enabled = true; + } else { + pr_err("Not enabling netconsole. Netpoll setup failed\n"); + if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC)) + /* only fail if dynamic reconfiguration is set, + * otherwise, keep the target in the list, but disabled. + */ + goto fail; + } populate_configfs_item(nt, cmdline_count); - nt->enabled = true; return nt;
Currently, netconsole discards targets that fail during initialization, causing two issues: 1) Inconsistency between target list and configfs entries * user pass cmdline0, cmdline1. If cmdline0 fails, then cmdline1 becomes cmdline0 in configfs. 2) Inability to manage failed targets from userspace * If user pass a target that fails with netpoll (interface not loaded at netcons initialization time, such as interface is a module), then the target will not exist in the configfs, so, user cannot re-enable or modify it from userspace. Failed targets are now added to the target list and configfs, but remain disabled until manually enabled or reconfigured. This change does not change the behaviour if CONFIG_NETCONSOLE_DYNAMIC is not set. CC: Aijay Adams <aijay@meta.com> Signed-off-by: Breno Leitao <leitao@debian.org> --- drivers/net/netconsole.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)