Re: [PATCH net-next v2 3/3] netconsole: Populate dynamic entry even if netpoll fails

[Breno Leitao]

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