From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paolo Abeni <pabeni@redhat.com>, Vlad Buslov <vladbu@nvidia.com>,
Oz Shlomo <ozsh@nvidia.com>,
kuba@kernel.org, davem@davemloft.net, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [GIT PULL] Networking for 6.0
Date: Thu, 4 Aug 2022 02:39:15 +0200 [thread overview]
Message-ID: <YusVM6B8bT6IOKdZ@salvia> (raw)
In-Reply-To: <CAHk-=wj59jR+pxYHmtf+OJvThEpYLNYLb9P5YkgCcBWDWzhdPQ@mail.gmail.com>
On Wed, Aug 03, 2022 at 05:27:07PM -0700, Linus Torvalds wrote:
> On Wed, Aug 3, 2022 at 5:11 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > For these two questions, this new Kconfig toggle was copied from:
> >
> > config NF_CONNTRACK_PROCFS
> > bool "Supply CT list in procfs (OBSOLETE)"
> > default y
> > depends on PROC_FS
> >
> > which is under:
> >
> > if NF_CONNTRACK
> >
> > but the copy and paste was missing this.
>
> Note that there's two problems with that
>
> (1) the NF_CONNTRACK_PROCFS thing is 'default y' because it *USED* to
> be unconditional, and was split up as a config option back in 2011.
>
> See commit 54b07dca6855 ("netfilter: provide config option to disable
> ancient procfs parts").
>
> IOW, that NF_CONNTRACK_PROCFS exists and defaults to on, not because
> people added new code and wanted to make it default, but because the
> code used to always be enabled if NF_CONNTRACK was enabled, and people
> wanted the option to *disable* it.
>
> That's when you do 'default y' - you take existing code that didn't
> originally have a question at all, and you make it optional. Then you
> use 'default y' so that people who used it don't get screwed in the
> process.
>
> (2) it didn't do the proper conditional on the feature it depended on.
>
> So let's not do copy-and-paste programming. The old Kconfig snippet
> had completely different rules, had completely different history, and
> completely different default values as a result.
>
> I realize that it's very easy to think of Kconfig as some
> not-very-important detail to just hook things up. But because it's
> front-facing to users, I do want people to think about it more than
> perhaps people otherwise would.
Agreed, it was a bad a idea to copy and paste it from
NF_CONNTRACK_PROCFS, this new toggle has nothing to do with it.
I'll take a closer look at any new Kconfig toggle coming in the
future to avoid issues like this.
Thanks for reviewing.
next prev parent reply other threads:[~2022-08-04 0:39 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-03 10:14 [GIT PULL] Networking for 6.0 Paolo Abeni
2022-08-03 23:35 ` Linus Torvalds
2022-08-04 9:43 ` Drewek, Wojciech
2022-08-03 23:50 ` pr-tracker-bot
2022-08-03 23:52 ` Linus Torvalds
2022-08-04 0:11 ` Pablo Neira Ayuso
2022-08-04 0:27 ` Linus Torvalds
2022-08-04 0:39 ` Pablo Neira Ayuso [this message]
2022-08-04 4:17 ` Linus Torvalds
2022-08-04 9:13 ` Kalle Valo
2022-08-05 14:22 ` Kalle Valo
2022-08-05 16:34 ` Linus Torvalds
2022-08-08 8:14 ` Kalle Valo
2022-08-08 18:33 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YusVM6B8bT6IOKdZ@salvia \
--to=pablo@netfilter.org \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=ozsh@nvidia.com \
--cc=pabeni@redhat.com \
--cc=torvalds@linux-foundation.org \
--cc=vladbu@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.