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

[Ryan Roberts]

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? 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.


> 
> [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
>>
>>
>>