Re: [PATCH v1 2/9] selftests/mm: Give scripts execute permission

[SeongJae Park]

On Fri, 14 Jul 2023 10:44:14 +0100 Ryan Roberts <ryan.roberts@xxxxxxx> wrote:

> On 13/07/2023 18:32, SeongJae Park wrote:
> > On Thu, 13 Jul 2023 16:39:33 +0200 David Hildenbrand <david@xxxxxxxxxx> wrote:
> > 
> >> On 13.07.23 15:54, Ryan Roberts wrote:
> >>> When run under run_vmtests.sh, test scripts were failing to run with
> >>> "permission denied" due to the scripts not being executable.
> >>>
> >>> It is also annoying not to be able to directly invoke run_vmtests.sh,
> >>> which is solved by giving also it the execute permission.
> >>>
> >>> Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>
> >>> ---
> >>>   tools/testing/selftests/mm/charge_reserved_hugetlb.sh  | 0
> >>>   tools/testing/selftests/mm/check_config.sh             | 0
> >>>   tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 0
> >>>   tools/testing/selftests/mm/run_vmtests.sh              | 0
> >>>   tools/testing/selftests/mm/test_hmm.sh                 | 0
> >>>   tools/testing/selftests/mm/test_vmalloc.sh             | 0
> >>>   tools/testing/selftests/mm/va_high_addr_switch.sh      | 0
> >>>   tools/testing/selftests/mm/write_hugetlb_memory.sh     | 0
> >>>   8 files changed, 0 insertions(+), 0 deletions(-)
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/check_config.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/hugetlb_reparenting_test.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/run_vmtests.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/test_hmm.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/test_vmalloc.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/va_high_addr_switch.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/write_hugetlb_memory.sh
> >>>
> >>> diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/check_config.sh b/tools/testing/selftests/mm/check_config.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/test_hmm.sh b/tools/testing/selftests/mm/test_hmm.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/test_vmalloc.sh b/tools/testing/selftests/mm/test_vmalloc.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/va_high_addr_switch.sh b/tools/testing/selftests/mm/va_high_addr_switch.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/write_hugetlb_memory.sh b/tools/testing/selftests/mm/write_hugetlb_memory.sh
> >>> old mode 100644
> >>> new mode 100755
> >>
> >> Sounds reasonable to me.
> >>
> >> Probably due to:
> >>
> >> commit baa489fabd01596d5426d6e112b34ba5fb59ab82
> >> Author: SeongJae Park <sj@xxxxxxxxxx>
> >> Date:   Tue Jan 3 18:07:53 2023 +0000
> >>
> >>      selftests/vm: rename selftests/vm to selftests/mm
> >>      
> >>      Rename selftets/vm to selftests/mm for being more consistent with the
> >>      code, documentation, and tools directories, and won't be confused with
> >>      virtual machines.
> >>
> >>
> >> and indeed, it contains
> >>
> >> diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> >> old mode 100755
> >> new mode 100644
> >> similarity index 100%
> >> rename from tools/testing/selftests/vm/run_vmtests.sh
> >> rename to tools/testing/selftests/mm/run_vmtests.sh
> > 
> > Thank you for tracking this and kindly Cc-ing me!  I'd like to clarify a little
> > bit more, though.  The permission change has made by the commit as you found.
> > Nevertheless, the submitted version[1] of the patch didn't change the
> > permission.  I guess the change was made while managing it via some file
> > permission unsupported patches management tool.
> > 
> > I had a similar issue with DAMON selftest and sent a patch restoring the
> > permission.  Greg suggested me to update the framework instead, to support such
> > management tool[2], so I made it[3].  It recently also merged into 5.15.y for
> > DAMON selftests[4].
> > 
> > I have no strong opinion about whether we need to keep the permission or it's
> > good to have no execute permission since kselftest framework supports it.  I
> > just wanted to clarify the events I've shown.  Please correct me if I missed or
> > wrong something.  Cc-ing Greg, since he might have an opinion.
> 
> Thanks for the detailed explanation. Are you effectively saying this patch will
> turn into a no-op once its been munged through the various patch management
> tools?

Depending on what tool maintainers that will pick this patch is using in what
way, I guess.

> That's disappointing because it's a pain to have to invoke everything
> though bash explicitly. Many other scripts manage to have the correct execute
> permission set (see everything in ./scripts for example).
> 
> Personally I'd rather keep this patch and try rather than proactively do a work
> around.

I don't have a strong opinion here, as mentioned before.  That said, I feel it
would be good to have a clear agreement or explanation about that, since I got
similar situation before[1].

[1] https://lore.kernel.org/damon/20230221175612.131555-1-sj@xxxxxxxxxx/


Thanks,
SJ

> 
> 
> > 
> > [1] https://lore.kernel.org/all/20230103180754.129637-5-sj@xxxxxxxxxx/
> > [2] https://lore.kernel.org/mm-commits/YRJisBs9AunccCD4@xxxxxxxxx/
> > [3] https://lore.kernel.org/all/20210810164534.25902-1-sj38.park@xxxxxxxxx/
> > [4] https://lore.kernel.org/stable/2023042743-cheesy-parasitic-206d@gregkh/
> > 
> > 
> > Thanks,
> > SJ
> > 
> >>
> >>
> >> Reviewed-by: David Hildenbrand <david@xxxxxxxxxx>
> >>
> >> -- 
> >> Cheers,
> >>
> >> David / dhildenb
> >>
> >>
> >>
> 
>