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

[Mike Rapoport <rppt@kernel.org>]

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

-- 
Sincerely yours,
Mike.