Re: [PATCH v3 1/4] tools/lib/mm: add shared file helpers

[Sarthak Sharma <sarthak.sharma@arm.com>]

On 5/26/26 2:31 PM, Mike Rapoport wrote:
> On Mon, May 25, 2026 at 11:59:32AM +0530, Sarthak Sharma wrote:
>> Hi Mike!
>>
>> On 5/24/26 10:36 PM, Mike Rapoport wrote:
>>> On Thu, 21 May 2026 16:47:58 +0530, Sarthak Sharma <sarthak.sharma@arm.com> wrote:
>>>
>>> Hi Sarthak,
>>>
>>>>
>>>> diff --git a/tools/lib/mm/file_utils.c b/tools/lib/mm/file_utils.c
>>>> new file mode 100644
>>>> index 000000000000..0f9322f2cf41
>>>> --- /dev/null
>>>> +++ b/tools/lib/mm/file_utils.c
>>>> @@ -0,0 +1,83 @@
>>>> [ ... skip 48 lines ... ]
>>>> +	saved_errno = errno;
>>>> +	close(fd);
>>>> +	errno = saved_errno;
>>>> +	if (numwritten < 0) {
>>>> +		fprintf(stderr, "%s write(%.*s) failed: %s\n",
>>>> +			path, (int)(buflen - 1), buf, strerror(errno));
>>>
>>> This would break TAP formatting for selftests.
>>
>> Yes, thanks for pointing it out.
>>
>>>
>>>> +		exit(EXIT_FAILURE);
>>>
>>> and while EXIT_FAILURE == KSFT_FAIL I'm not sure it's robust enough.
>>
>> I used EXIT_FAILURE here because the helper is moving out of selftests
>> and should not include kselftest.h anymore. The helper already
>> terminated the process on these paths, so I tried to preserve that
>> behavior while removing the ksft dependency.
> 
> In mm selftests a failure to update a /proc or /sysfs file meant there is
> no point to continue the test. But if we make it a generic helper for
> potentially broader use than mm selftests, exit() on failure is too harsh.

Okay yeah, this makes sense.

>  
>> We can change this to return errors instead of calling exit() and update
>> the selftest callers to report failures through the ksft_* helpers. I
>> agree this is cleaner, but it would grow the series a bit.
>>
>> If you feel strongly, I can include these changes in v4. Otherwise I
>> feel we can handle it separately later to avoid growing this series.
> 
> There are not that many callers of write_file() and write_num().
> I think a patch that makes them return an error rather than exit() can go
> before moving these functions to lib. 
> 

So I will add a patch before the move that makes read_file(),
write_file(), read_num() and write_num() return errors instead of
exiting and update the existing selftest callers to report those
failures via ksft_* helpers.

For hugepage_settings.c, I’d prefer to keep the existing fail fast
behaviour in this series. After this series, the users are still mm
selftests and the new tools/mm/gup_bench tool and for those users a
failure to read/write THP or HugeTLB state is fatal to the operation
being attempted.

Converting the full hugepage_settings API to return errors would be a
larger follow-up, because many of its helpers are used throughout mm
selftests. I can handle that in a separate series unless you think it
should be folded into this one as well.

>>>>
>>>> diff --git a/tools/testing/selftests/mm/hugepage_settings.c b/tools/testing/selftests/mm/hugepage_settings.c
>>>> index 2eab2110ac6a..5e947abb7425 100644
>>>> --- a/tools/testing/selftests/mm/hugepage_settings.c
>>>> +++ b/tools/testing/selftests/mm/hugepage_settings.c
>>>> @@ -8,8 +8,9 @@
>>>>  #include <stdlib.h>
>>>>  #include <string.h>
>>>>  #include <unistd.h>
>>>> +#include <mm/file_utils.h>
>>>>  
>>>> -#include "vm_util.h"
>>>
>>> I think it would be fine to include file_utils.h in vm_utils.h and avoid
>>> further churn.
>>
>> Okay, I'll change this.
>