| Message ID | 20240809122007.1220219-3-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: Do not include <linux/*.h> from host programs (+ extra clean-up) | expand |
On Aug 9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote: > > This tool is only used in security/selinux/Makefile. > > There is no reason to keep it under scripts/. > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > --- > scripts/remove-stale-files | 3 +++ > scripts/selinux/Makefile | 2 +- > scripts/selinux/genheaders/.gitignore | 2 -- > scripts/selinux/genheaders/Makefile | 3 --- > security/selinux/.gitignore | 1 + > security/selinux/Makefile | 7 +++++-- > .../selinux/genheaders => security/selinux}/genheaders.c | 0 > 7 files changed, 10 insertions(+), 8 deletions(-) > delete mode 100644 scripts/selinux/genheaders/.gitignore > delete mode 100644 scripts/selinux/genheaders/Makefile > rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%) As long as there is no harm in keeping genheaders under scripts/selinux, and based on your cover letter it would appear that there is no problem with the current location, I would prefer to keep it where it currently lives. -- paul-moore.com
On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote: > > On Aug 9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > This tool is only used in security/selinux/Makefile. > > > > There is no reason to keep it under scripts/. > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > > --- > > scripts/remove-stale-files | 3 +++ > > scripts/selinux/Makefile | 2 +- > > scripts/selinux/genheaders/.gitignore | 2 -- > > scripts/selinux/genheaders/Makefile | 3 --- > > security/selinux/.gitignore | 1 + > > security/selinux/Makefile | 7 +++++-- > > .../selinux/genheaders => security/selinux}/genheaders.c | 0 > > 7 files changed, 10 insertions(+), 8 deletions(-) > > delete mode 100644 scripts/selinux/genheaders/.gitignore > > delete mode 100644 scripts/selinux/genheaders/Makefile > > rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%) > > As long as there is no harm in keeping genheaders under scripts/selinux, > and based on your cover letter it would appear that there is no problem > with the current location, I would prefer to keep it where it currently > lives. 'make clean' is meant to clean up the tree, but keep build artifacts necessary for building external modules. See the help message: clean - Remove most generated files but keep the config and enough build support to build external modules 'make clean' does not clean up under scripts/ because tools located scripts/ are used in tree-wide and often used for external modules as well. So, scripts/selinux/genheaders/genheaders is left over. genheaders is locally used in security/selinux/. 'make clean' will properly clean up security/selinux/genheaders.
On Fri, Sep 6, 2024 at 11:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote: > > On Aug 9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > This tool is only used in security/selinux/Makefile. > > > > > > There is no reason to keep it under scripts/. > > > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > > > --- > > > scripts/remove-stale-files | 3 +++ > > > scripts/selinux/Makefile | 2 +- > > > scripts/selinux/genheaders/.gitignore | 2 -- > > > scripts/selinux/genheaders/Makefile | 3 --- > > > security/selinux/.gitignore | 1 + > > > security/selinux/Makefile | 7 +++++-- > > > .../selinux/genheaders => security/selinux}/genheaders.c | 0 > > > 7 files changed, 10 insertions(+), 8 deletions(-) > > > delete mode 100644 scripts/selinux/genheaders/.gitignore > > > delete mode 100644 scripts/selinux/genheaders/Makefile > > > rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%) > > > > As long as there is no harm in keeping genheaders under scripts/selinux, > > and based on your cover letter it would appear that there is no problem > > with the current location, I would prefer to keep it where it currently > > lives. > > 'make clean' is meant to clean up the tree, but keep > build artifacts necessary for building external modules. > > See the help message: > > clean - Remove most generated files but keep the config and > enough build support to build external modules > > 'make clean' does not clean up under scripts/ > because tools located scripts/ are used in tree-wide > and often used for external modules as well. > > So, scripts/selinux/genheaders/genheaders is left over. > > genheaders is locally used in security/selinux/. > > 'make clean' will properly clean up security/selinux/genheaders. Your last sentence is confusing and doesn't align with the rest of your email, please clarify. Regardless, this sort of explanation is what one needs to put in the commit description, a simple "There is no reason to keep it under scripts/" isn't sufficient. Patches like this need to provide a well defined reason to justify the code churn.
On Sat, Sep 7, 2024 at 12:37 AM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Sep 6, 2024 at 11:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote: > > > On Aug 9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > > > This tool is only used in security/selinux/Makefile. > > > > > > > > There is no reason to keep it under scripts/. > > > > > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > > > > --- > > > > scripts/remove-stale-files | 3 +++ > > > > scripts/selinux/Makefile | 2 +- > > > > scripts/selinux/genheaders/.gitignore | 2 -- > > > > scripts/selinux/genheaders/Makefile | 3 --- > > > > security/selinux/.gitignore | 1 + > > > > security/selinux/Makefile | 7 +++++-- > > > > .../selinux/genheaders => security/selinux}/genheaders.c | 0 > > > > 7 files changed, 10 insertions(+), 8 deletions(-) > > > > delete mode 100644 scripts/selinux/genheaders/.gitignore > > > > delete mode 100644 scripts/selinux/genheaders/Makefile > > > > rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%) > > > > > > As long as there is no harm in keeping genheaders under scripts/selinux, > > > and based on your cover letter it would appear that there is no problem > > > with the current location, I would prefer to keep it where it currently > > > lives. > > > > 'make clean' is meant to clean up the tree, but keep > > build artifacts necessary for building external modules. > > > > See the help message: > > > > clean - Remove most generated files but keep the config and > > enough build support to build external modules > > > > 'make clean' does not clean up under scripts/ > > because tools located scripts/ are used in tree-wide > > and often used for external modules as well. > > > > So, scripts/selinux/genheaders/genheaders is left over. > > > > genheaders is locally used in security/selinux/. > > > > 'make clean' will properly clean up security/selinux/genheaders. > > Your last sentence is confusing and doesn't align with the rest of > your email, please clarify. I do not understand what was unclear. 'make clean' cannot clean the current path, scripts/selinux/genheaders/genheaders. 'make clean' can clean the proposed path, security/selinux/genheaders. genheaders is only used during the kernel build. When you run 'make clean', there is no reason to keep it for external modules. Thus, it should move to the directory path that 'make clean' can handle. Clearer? > > Regardless, this sort of explanation is what one needs to put in the > commit description, a simple "There is no reason to keep it under > scripts/" isn't sufficient. Patches like this need to provide a well > defined reason to justify the code churn. > > -- > paul-moore.com
On Fri, Sep 6, 2024 at 12:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > On Sat, Sep 7, 2024 at 12:37 AM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Sep 6, 2024 at 11:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Aug 9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > > > > > This tool is only used in security/selinux/Makefile. > > > > > > > > > > There is no reason to keep it under scripts/. > > > > > > > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > > > > > --- > > > > > scripts/remove-stale-files | 3 +++ > > > > > scripts/selinux/Makefile | 2 +- > > > > > scripts/selinux/genheaders/.gitignore | 2 -- > > > > > scripts/selinux/genheaders/Makefile | 3 --- > > > > > security/selinux/.gitignore | 1 + > > > > > security/selinux/Makefile | 7 +++++-- > > > > > .../selinux/genheaders => security/selinux}/genheaders.c | 0 > > > > > 7 files changed, 10 insertions(+), 8 deletions(-) > > > > > delete mode 100644 scripts/selinux/genheaders/.gitignore > > > > > delete mode 100644 scripts/selinux/genheaders/Makefile > > > > > rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%) > > > > > > > > As long as there is no harm in keeping genheaders under scripts/selinux, > > > > and based on your cover letter it would appear that there is no problem > > > > with the current location, I would prefer to keep it where it currently > > > > lives. > > > > > > 'make clean' is meant to clean up the tree, but keep > > > build artifacts necessary for building external modules. > > > > > > See the help message: > > > > > > clean - Remove most generated files but keep the config and > > > enough build support to build external modules > > > > > > 'make clean' does not clean up under scripts/ > > > because tools located scripts/ are used in tree-wide > > > and often used for external modules as well. > > > > > > So, scripts/selinux/genheaders/genheaders is left over. > > > > > > genheaders is locally used in security/selinux/. > > > > > > 'make clean' will properly clean up security/selinux/genheaders. > > > > Your last sentence is confusing and doesn't align with the rest of > > your email, please clarify. > > I do not understand what was unclear. Near the start of your email you stated: "'make clean' does not clean up under scripts/". However you ended your email with "'make clean' will properly clean up security/selinux/genheaders" which seems contradictory to your initial statement; I was guessing that you were implying that moving the genheaders script will allow `make clean` to work properly, but you explicitly included the old/existing location of security/selinux/genheaders directory in your comment which didn't support that guess. Your latest reply makes it a bit more clear.
On Sat, Sep 7, 2024 at 1:23 AM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Sep 6, 2024 at 12:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Sat, Sep 7, 2024 at 12:37 AM Paul Moore <paul@paul-moore.com> wrote: > > > On Fri, Sep 6, 2024 at 11:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > On Aug 9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > > > > > > > This tool is only used in security/selinux/Makefile. > > > > > > > > > > > > There is no reason to keep it under scripts/. > > > > > > > > > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > > > > > > --- > > > > > > scripts/remove-stale-files | 3 +++ > > > > > > scripts/selinux/Makefile | 2 +- > > > > > > scripts/selinux/genheaders/.gitignore | 2 -- > > > > > > scripts/selinux/genheaders/Makefile | 3 --- > > > > > > security/selinux/.gitignore | 1 + > > > > > > security/selinux/Makefile | 7 +++++-- > > > > > > .../selinux/genheaders => security/selinux}/genheaders.c | 0 > > > > > > 7 files changed, 10 insertions(+), 8 deletions(-) > > > > > > delete mode 100644 scripts/selinux/genheaders/.gitignore > > > > > > delete mode 100644 scripts/selinux/genheaders/Makefile > > > > > > rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%) > > > > > > > > > > As long as there is no harm in keeping genheaders under scripts/selinux, > > > > > and based on your cover letter it would appear that there is no problem > > > > > with the current location, I would prefer to keep it where it currently > > > > > lives. > > > > > > > > 'make clean' is meant to clean up the tree, but keep > > > > build artifacts necessary for building external modules. > > > > > > > > See the help message: > > > > > > > > clean - Remove most generated files but keep the config and > > > > enough build support to build external modules > > > > > > > > 'make clean' does not clean up under scripts/ > > > > because tools located scripts/ are used in tree-wide > > > > and often used for external modules as well. > > > > > > > > So, scripts/selinux/genheaders/genheaders is left over. > > > > > > > > genheaders is locally used in security/selinux/. > > > > > > > > 'make clean' will properly clean up security/selinux/genheaders. > > > > > > Your last sentence is confusing and doesn't align with the rest of > > > your email, please clarify. > > > > I do not understand what was unclear. > > Near the start of your email you stated: "'make clean' does not clean > up under scripts/". However you ended your email with "'make clean' > will properly clean up security/selinux/genheaders" which seems > contradictory to your initial statement; I was guessing that you were > implying that moving the genheaders script will allow `make clean` to > work properly, but you explicitly included the old/existing location > of security/selinux/genheaders directory in your comment which didn't > support that guess. OK, now I understand. I meant this: With this patch applied, 'make clean' will properly clean up security/selinux/genheaders. > Your latest reply makes it a bit more clear. So, are you ok with the following commit description, which I proposed in another thread? --------------->8-------------------- selinux: move genheaders to security/selinux/ This tool is only used in security/selinux/Makefile. Move it to security/selinux/ so that 'make clean' can clean it up. Please note 'make clean' does not visit scripts/ because tools under scripts/ are often used for external module builds. Obviously, genheaders is not the case here. --------------->8-------------------- -- Best Regards Masahiro Yamada
On Fri, Sep 6, 2024 at 12:38 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Sat, Sep 7, 2024 at 1:23 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Fri, Sep 6, 2024 at 12:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > On Sat, Sep 7, 2024 at 12:37 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Fri, Sep 6, 2024 at 11:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Aug 9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > > > > > > > > > This tool is only used in security/selinux/Makefile. > > > > > > > > > > > > > > There is no reason to keep it under scripts/. > > > > > > > > > > > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > > > > > > > --- > > > > > > > scripts/remove-stale-files | 3 +++ > > > > > > > scripts/selinux/Makefile | 2 +- > > > > > > > scripts/selinux/genheaders/.gitignore | 2 -- > > > > > > > scripts/selinux/genheaders/Makefile | 3 --- > > > > > > > security/selinux/.gitignore | 1 + > > > > > > > security/selinux/Makefile | 7 +++++-- > > > > > > > .../selinux/genheaders => security/selinux}/genheaders.c | 0 > > > > > > > 7 files changed, 10 insertions(+), 8 deletions(-) > > > > > > > delete mode 100644 scripts/selinux/genheaders/.gitignore > > > > > > > delete mode 100644 scripts/selinux/genheaders/Makefile > > > > > > > rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%) > > > > > > > > > > > > As long as there is no harm in keeping genheaders under scripts/selinux, > > > > > > and based on your cover letter it would appear that there is no problem > > > > > > with the current location, I would prefer to keep it where it currently > > > > > > lives. > > > > > > > > > > 'make clean' is meant to clean up the tree, but keep > > > > > build artifacts necessary for building external modules. > > > > > > > > > > See the help message: > > > > > > > > > > clean - Remove most generated files but keep the config and > > > > > enough build support to build external modules > > > > > > > > > > 'make clean' does not clean up under scripts/ > > > > > because tools located scripts/ are used in tree-wide > > > > > and often used for external modules as well. > > > > > > > > > > So, scripts/selinux/genheaders/genheaders is left over. > > > > > > > > > > genheaders is locally used in security/selinux/. > > > > > > > > > > 'make clean' will properly clean up security/selinux/genheaders. > > > > > > > > Your last sentence is confusing and doesn't align with the rest of > > > > your email, please clarify. > > > > > > I do not understand what was unclear. > > > > Near the start of your email you stated: "'make clean' does not clean > > up under scripts/". However you ended your email with "'make clean' > > will properly clean up security/selinux/genheaders" which seems > > contradictory to your initial statement; I was guessing that you were > > implying that moving the genheaders script will allow `make clean` to > > work properly, but you explicitly included the old/existing location > > of security/selinux/genheaders directory in your comment which didn't > > support that guess. > > > OK, now I understand. > > > I meant this: > > > With this patch applied, 'make clean' will properly clean up > security/selinux/genheaders. > > > > > Your latest reply makes it a bit more clear. > > > > So, are you ok with the following commit description, > which I proposed in another thread? > > > > --------------->8-------------------- > selinux: move genheaders to security/selinux/ > > This tool is only used in security/selinux/Makefile. > > Move it to security/selinux/ so that 'make clean' can clean it up. > > Please note 'make clean' does not visit scripts/ because tools under > scripts/ are often used for external module builds. Obviously, genheaders > is not the case here. > --------------->8-------------------- Yes, thank you.
diff --git a/scripts/remove-stale-files b/scripts/remove-stale-files index f38d26b78c2a..4e7d25668a98 100755 --- a/scripts/remove-stale-files +++ b/scripts/remove-stale-files @@ -20,4 +20,7 @@ set -e # yard. Stale files stay in this file for a while (for some release cycles?), # then will be really dead and removed from the code base entirely. +# moved to security/selinux/genheaders +rm -f scripts/selinux/genheaders/genheaders + rm -f *.spec diff --git a/scripts/selinux/Makefile b/scripts/selinux/Makefile index 59494e14989b..4b1308fa5732 100644 --- a/scripts/selinux/Makefile +++ b/scripts/selinux/Makefile @@ -1,2 +1,2 @@ # SPDX-License-Identifier: GPL-2.0-only -subdir-y := mdp genheaders +subdir-y := mdp diff --git a/scripts/selinux/genheaders/.gitignore b/scripts/selinux/genheaders/.gitignore deleted file mode 100644 index 5fcadd307908..000000000000 --- a/scripts/selinux/genheaders/.gitignore +++ /dev/null @@ -1,2 +0,0 @@ -# SPDX-License-Identifier: GPL-2.0-only -genheaders diff --git a/scripts/selinux/genheaders/Makefile b/scripts/selinux/genheaders/Makefile deleted file mode 100644 index 866f60e78882..000000000000 --- a/scripts/selinux/genheaders/Makefile +++ /dev/null @@ -1,3 +0,0 @@ -# SPDX-License-Identifier: GPL-2.0 -hostprogs-always-y += genheaders -HOST_EXTRACFLAGS += -I$(srctree)/security/selinux/include diff --git a/security/selinux/.gitignore b/security/selinux/.gitignore index 168fae13ca5a..01c0df8ab009 100644 --- a/security/selinux/.gitignore +++ b/security/selinux/.gitignore @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only av_permissions.h flask.h +/genheaders diff --git a/security/selinux/Makefile b/security/selinux/Makefile index c47519ed8156..86f0575f670d 100644 --- a/security/selinux/Makefile +++ b/security/selinux/Makefile @@ -36,7 +36,10 @@ quiet_cmd_genhdrs = GEN $(addprefix $(obj)/,$(genhdrs)) # see the note above, replace the $targets and 'flask.h' rule with the lines # below: # targets += $(genhdrs) -# $(addprefix $(obj)/,$(genhdrs)) &: scripts/selinux/... +# $(addprefix $(obj)/,$(genhdrs)) &: $(obj)/genheaders FORCE targets += flask.h -$(obj)/flask.h: scripts/selinux/genheaders/genheaders FORCE +$(obj)/flask.h: $(obj)/genheaders FORCE $(call if_changed,genhdrs) + +hostprogs := genheaders +HOST_EXTRACFLAGS += -I$(srctree)/security/selinux/include diff --git a/scripts/selinux/genheaders/genheaders.c b/security/selinux/genheaders.c similarity index 100% rename from scripts/selinux/genheaders/genheaders.c rename to security/selinux/genheaders.c
This tool is only used in security/selinux/Makefile. There is no reason to keep it under scripts/. Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- scripts/remove-stale-files | 3 +++ scripts/selinux/Makefile | 2 +- scripts/selinux/genheaders/.gitignore | 2 -- scripts/selinux/genheaders/Makefile | 3 --- security/selinux/.gitignore | 1 + security/selinux/Makefile | 7 +++++-- .../selinux/genheaders => security/selinux}/genheaders.c | 0 7 files changed, 10 insertions(+), 8 deletions(-) delete mode 100644 scripts/selinux/genheaders/.gitignore delete mode 100644 scripts/selinux/genheaders/Makefile rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%)