[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZsWjpuoszvApM1I0@gmail.com>
Date: Wed, 21 Aug 2024 01:21:58 -0700
From: Breno Leitao <leitao@...ian.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Aijay Adams <aijay@...a.com>
Subject: Re: [PATCH net-next v2 3/3] netconsole: Populate dynamic entry even
if netpoll fails
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
Powered by blists - more mailing lists