Re: [LTP] [PATCH v5] statvfs01: Convert to new LTP API

Richard Palethorpe
2022-12-22T09:59:23Z

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Avinesh, Richie,
>
> Generally LGTM, thanks for fixing exfat and vfat.
>
>> > + toolong_fname = SAFE_MALLOC(buf.f_namemax + 1);
> However, length could be smaller:
> Instead using buf.f_namemax + 1 (1531) also for exfat and vfat,
> invalid length is already buf.f_namemax / NLS_MAX_CHARSET_SIZE + 1
> (256).
>
>> > + if (fs_type == TST_VFAT_MAGIC || fs_type == TST_EXFAT_MAGIC)
>> > + valid_fname = SAFE_MALLOC(buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1);
>> > + else
>> > + valid_fname = SAFE_MALLOC(buf.f_namemax - 1);

There is also a memory leak when running with -I. You could just use an
4Kb (PATH_MAX) static buffer as the name. If we find an FS that allows
longer names then we can increase it.

We could also use a guarded buffer (specified in tst_test).

>
>> > - if (TEST_RETURN == -1) {
>> > - tst_resm(TFAIL | TTERRNO, "statvfs(%s, ...) failed",
>> > - TEST_PATH);
>> > - } else {
>> > - tst_resm(TPASS, "statvfs(%s, ...) passed", TEST_PATH);
>> > - }
>> > + memset(toolong_fname, 'b', buf.f_namemax + 1);
>> > + if (fs_type == TST_VFAT_MAGIC || fs_type == TST_EXFAT_MAGIC)
>> > + memset(valid_fname, 'a', buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1);
>> > + else
>> > + memset(valid_fname, 'a', buf.f_namemax - 1);
> Also valid length is for buf.f_namemax, not buf.f_namemax - 1. I guess -1 is for
> \0 (NULL terminator), but tests work even with just buf.f_namemax.
>
> Also adding variable to hold the length makes source more readable.
> How about this?
>
> struct statvfs buf;
> char *valid_fname, *toolong_fname;
> long fs_type;
> long valid_len;
>
> fs_type = tst_fs_type(TEST_PATH);
>
> TST_EXP_PASS(statvfs(TEST_PATH, &buf));
>
> valid_len = buf.f_namemax;
> if (fs_type == TST_VFAT_MAGIC || fs_type == TST_EXFAT_MAGIC)
> valid_len = buf.f_namemax / NLS_MAX_CHARSET_SIZE;
>
> valid_fname = SAFE_MALLOC(valid_len);
> memset(valid_fname, 'a', valid_len);
>
> toolong_fname = SAFE_MALLOC(valid_len + 1);
> memset(toolong_fname, 'b', valid_len + 1);
>
> Final diff is below.
>
> Kind regards,
> Petr
>
> diff --git testcases/kernel/syscalls/statvfs/statvfs01.c testcases/kernel/syscalls/statvfs/statvfs01.c
> index 034835da7d..f357855eb1 100644
> --- testcases/kernel/syscalls/statvfs/statvfs01.c
> +++ testcases/kernel/syscalls/statvfs/statvfs01.c
> @@ -25,22 +25,21 @@ static void run(void)
> struct statvfs buf;
> char *valid_fname, *toolong_fname;
> long fs_type;
> + long valid_len;
>
> fs_type = tst_fs_type(TEST_PATH);
>
> TST_EXP_PASS(statvfs(TEST_PATH, &buf));
>
> - toolong_fname = SAFE_MALLOC(buf.f_namemax + 1);
> + valid_len = buf.f_namemax;
> if (fs_type == TST_VFAT_MAGIC || fs_type == TST_EXFAT_MAGIC)
> - valid_fname = SAFE_MALLOC(buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1);
> - else
> - valid_fname = SAFE_MALLOC(buf.f_namemax - 1);
> + valid_len = buf.f_namemax / NLS_MAX_CHARSET_SIZE;
>
> - memset(toolong_fname, 'b', buf.f_namemax + 1);
> - if (fs_type == TST_VFAT_MAGIC || fs_type == TST_EXFAT_MAGIC)
> - memset(valid_fname, 'a', buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1);
> - else
> - memset(valid_fname, 'a', buf.f_namemax - 1);
> + valid_fname = SAFE_MALLOC(valid_len);
> + memset(valid_fname, 'a', valid_len);
> +
> + toolong_fname = SAFE_MALLOC(valid_len + 1);
> + memset(toolong_fname, 'b', valid_len + 1);
>
> TST_EXP_FD(creat(valid_fname, 0444));
> SAFE_CLOSE(TST_RET);

--
Thank you,
Richard.