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

[Breno Leitao <leitao@debian.org>]

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