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

Avinesh Kumar
2022-12-23T14:41:01Z

Hi Petr, Richie,

Thank you for reviewing again and providing more improvement suggestions, I've
made the changes in v6.

Thanks,
Avinesh

On Thursday, December 22, 2022 3:23:35 PM IST Richard Palethorpe wrote:
> 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);
>
>
>