Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Jul 2023 01:49:36 -0700
From:   Breno Leitao <leitao@debian.org>
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>, sergey.senozhatsky@gmail.com,
        pmladek@suse.com, tj@kernel.org, stephen@networkplumber.org,
        Dave Jones <davej@codemonkey.org.uk>,
        "open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] netconsole: Append kernel version to message
Message-ID: <ZKvGIGbWa751sfTA@gmail.com>
References: <20230707132911.2033870-1-leitao@debian.org>
 <20230707161050.61ec46a8@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230707161050.61ec46a8@kernel.org>
Precedence: bulk

On Fri, Jul 07, 2023 at 04:10:50PM -0700, Jakub Kicinski wrote:
> On Fri,  7 Jul 2023 06:29:11 -0700 Breno Leitao wrote:
> > Create a new netconsole runtime option that prepends the kernel version in
> > the netconsole message. This is useful to map kernel messages to kernel
> > version in a simple way, i.e., without checking somewhere which kernel
> > version the host that sent the message is using.
> > 
> > If this option is selected, then the "<release>," is prepended before the
> > netconsole message. This is an example of a netconsole output, with
> > release feature enabled:
> > 
> > 	6.4.0-01762-ga1ba2ffe946e;12,426,112883998,-;this is a test
> > 
> > Calvin Owens send a RFC about this problem in 2016[1], but his
> > approach was a bit more intrusive, changing the printk subsystem. This
> > approach is lighter, and just append the information in the last mile,
> > just before netconsole push the message to netpoll.
> > 
> > [1] Link: https://lore.kernel.org/all/51047c0f6e86abcb9ee13f60653b6946f8fcfc99.1463172791.git.calvinowens@fb.com/
> > 
> > Cc: Dave Jones <davej@codemonkey.org.uk>
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> Looks good! net-next is closed for the duration of the merge window 
> so you'll need to repost next week, and please put [PATCH net-next v3]
> as the subject prefix while at it.
> 
> > @@ -332,6 +350,11 @@ static ssize_t enabled_store(struct config_item *item,
> >  	}
> >  
> >  	if (enabled) {	/* true */
> > +		if (nt->release && !nt->extended) {
> > +			pr_err("release feature requires extended log message\n");
> > +			goto out_unlock;
> > +		}
> 
> This is the only bit that gave me pause - when parsing the command line
> you ignore release if extended is not set (with an error/warning).
> Does it make sense to be consistent and do the same thing here? 
> Or enabling at runtime is fundamentally different?

That is a good point, this patch ignores "release" if extended feature
is disabled in the command line, but, fails if "release" is set and
extended is not.

Looking at the other behaviours (netpoll parsing_ in the code, I think
the best approach is to also fail on both cases.

I'll fix it in v3.

Thanks for the review!