Re: [LTP] [PATCH 1/1] tst_tmpdir: Remove possible double/trailing slashes from TMPDIR

[Petr Vorel <pvorel@suse.cz>]

Hi Li,

> On Wed, Apr 19, 2023 at 2:47 PM Li Wang <liwang@redhat.com> wrote:

> > Hi Petr,

> > On Thu, Apr 13, 2023 at 7:14 PM Petr Vorel <pvorel@suse.cz> wrote:

> >> Similarly to previous commit, which normalized TMPDIR for shell API,
> >> do the same for C API.

> >> Unlike for shell API, here we modify $TMPDIR directly, because
> >> tst_get_tmpdir_root() is used o more places.

> >> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> >> ---
> >> Follow up of the same change in shell API:
> >> https://lore.kernel.org/ltp/20230412073953.1983857-1-pvorel@suse.cz/

> >> Kind regards,
> >> Petr

> >>  lib/tst_tmpdir.c | 16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)

> >> diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
> >> index b73b5c66f..8db5c47e8 100644
> >> --- a/lib/tst_tmpdir.c
> >> +++ b/lib/tst_tmpdir.c
> >> @@ -124,16 +124,28 @@ char *tst_get_tmpdir(void)

> >>  const char *tst_get_tmpdir_root(void)
> >>  {
> >> -       const char *env_tmpdir = getenv("TMPDIR");
> >> +       char *env_tmpdir = getenv("TMPDIR");


> > It seems that modifying the environment variables is generally
> > not a good practice.

> > The getenv() function returns a pointer to the value of an
> > environment variable, which is stored in the memory managed
> > by the system. Any attempt to modify this memory directly can
> > cause unexpected behavior or even crash the program.

> > Instead of modifying the return value of getenv(), it is recommended
> > to create a copy of the value and modify the copy instead.

Do you mean to use strdup()?

Also man getenv(3) says:

       As typically implemented, getenv() returns a pointer to a string
       within the environment list.  The caller must take care not to
       modify this string, since that would change the environment of
       the process.

=> I would not mind $TMPDIR got updated in the environment.

> Btw, the wise method is to use setenv() function to reset
> environment variables if really needed.

Well, I don't know any C test which needs it (only NFS tests which are shell
tests). But I wanted to have the same behavior in both APIs.

> This is a different part of shell API I have to say.

Yes, the behavior is slightly different from shell API [1],
where it modifies $TST_TMPDIR (keep $TMPDIR untouched).


> > Or, the simplest way I guess is just TBROK and tell users why
> > this TMPDIR is unusable.

If you prefer it's better to TBROK on:
* double slashes
* trailing slash

I can do that. But at least on trailing slash looks to me quite strict.

Whatever path we choose, I'd need also to update docs. BTW the need
to absolute path for TMPDIR is only in C - shell happily takes relative path
and IMHO it's not documented.

Kind regards,
Petr

[1] https://patchwork.ozlabs.org/project/ltp/patch/20230412073953.1983857-1-pvorel@suse.cz/

> >> -       if (!env_tmpdir)
> >> +       if (env_tmpdir) {
> >> +               /* remove duplicate slashes */
> >> +               for (char *p = env_tmpdir, *q = env_tmpdir; *q;) {
> >> +                       if (*++q != '/' || *p != '/')
> >> +                               *++p = *q;
> >> +               }
> >> +               /* Remove slash on the last place  */
> >> +               size_t last = strlen(env_tmpdir)-1;
> >> +               if (env_tmpdir[last] == '/')
> >> +                       env_tmpdir[last] = '\0';
> >> +       } else {
> >>                 env_tmpdir = TEMPDIR;
> >> +       }

> >>         if (env_tmpdir[0] != '/') {
> >>                 tst_brkm(TBROK, NULL, "You must specify an absolute "
> >>                                 "pathname for environment variable
> >> TMPDIR");
> >>                 return NULL;
> >>         }
> >> +
> >>         return env_tmpdir;
> >>  }

> >> --
> >> 2.40.0



> > --
> > Regards,
> > Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp