| Message ID | 20210615163307.10755-3-pvorel@suse.cz |
|---|---|
| State | Rejected |
| Headers | show |
| Series | C, shell API: Add $LTPROOT/testcases/bin into PATH | expand |
Hi! > diff --git a/lib/tst_test.c b/lib/tst_test.c > index 36a4809c7..14a739eb5 100644 > --- a/lib/tst_test.c > +++ b/lib/tst_test.c > @@ -1149,15 +1149,31 @@ static unsigned long long get_time_ms(void) > static void add_paths(void) > { > char *old_path = getenv("PATH"); > + const char *ltproot = getenv("LTPROOT"); > const char *start_dir; > - char *new_path; > + char *bin_dir, *new_path = NULL; > > start_dir = tst_get_startwd(); > > + /* : - current directory */ > if (old_path) > - SAFE_ASPRINTF(&new_path, "%s::%s", old_path, start_dir); > + SAFE_ASPRINTF(&new_path, "%s:", old_path); > else > - SAFE_ASPRINTF(&new_path, "::%s", start_dir); > + strcat(new_path, ":"); I personally think that strcat() function is broken be desing and that we should avoid using it. Also in this place is guaranteed SEGFAULT since you strcat() to NULL. > + /* empty for library C API tests */ > + if (strlen(start_dir) > 0) > + SAFE_ASPRINTF(&new_path, "%s:%s", new_path, start_dir); ^ This is a memory leak. As far as I can tell the asprintf() does not free th strp if non-NULL. > + if (ltproot) { > + SAFE_ASPRINTF(&bin_dir, "%s/testcases/bin", ltproot); > + > + if (access(bin_dir, F_OK) == -1) > + tst_res(TWARN, "'%s' does not exist, is $LTPROOT set correctly?", > + bin_dir); > + else > + SAFE_ASPRINTF(&new_path, "%s:%s", new_path, bin_dir); ^ And this as well. > + } I guess that we can also fairly simplify the code by expecting that PATH is never unset from the start, maybe we should just check it and WARN if it's not. Also we can assume that if LTPROOT is set we do not have to add the start_dir since the start_dir is only useful when tests are executed from the git checkout. Something as: const char *old_path = getenv("PATH"); const char *ltproot = getenv("LTPROOT"); const char *start_dir = tst_get_startwd(); char *new_path; if (!old_path) { tst_res(TWARN, "\$PATH not set!"); return; } if (ltproot) SAFE_ASPRINTF(&new_path, "%s::%s/testcases/bin/", old_path, ltproot); else SAFE_ASPRINTF(&new_path, "%s::%s", old_path, start_dir); > SAFE_SETENV("PATH", new_path, 1); > free(new_path); > -- > 2.32.0 >
Hi Cyril, > I guess that we can also fairly simplify the code by expecting that PATH > is never unset from the start, maybe we should just check it and WARN if > it's not. Also we can assume that if LTPROOT is set we do not have to > add the start_dir since the start_dir is only useful when tests are > executed from the git checkout. I'm sorry to sent patchset with full of bugs, thanks for all your explanation. While your suggestion could work, it's a question if my effort help to anything (as Li noted). My intend was to require only LTPROOT, but even we expect script/binary is called by full path (otherwise adjusting PATH would be required anyway) fix work only for C API. tst_test.sh PATH is missing for shell API :(. (discussed with Li under 3rd patch) Kind regards, Petr
diff --git a/lib/tst_test.c b/lib/tst_test.c index 36a4809c7..14a739eb5 100644 --- a/lib/tst_test.c +++ b/lib/tst_test.c @@ -1149,15 +1149,31 @@ static unsigned long long get_time_ms(void) static void add_paths(void) { char *old_path = getenv("PATH"); + const char *ltproot = getenv("LTPROOT"); const char *start_dir; - char *new_path; + char *bin_dir, *new_path = NULL; start_dir = tst_get_startwd(); + /* : - current directory */ if (old_path) - SAFE_ASPRINTF(&new_path, "%s::%s", old_path, start_dir); + SAFE_ASPRINTF(&new_path, "%s:", old_path); else - SAFE_ASPRINTF(&new_path, "::%s", start_dir); + strcat(new_path, ":"); + + /* empty for library C API tests */ + if (strlen(start_dir) > 0) + SAFE_ASPRINTF(&new_path, "%s:%s", new_path, start_dir); + + if (ltproot) { + SAFE_ASPRINTF(&bin_dir, "%s/testcases/bin", ltproot); + + if (access(bin_dir, F_OK) == -1) + tst_res(TWARN, "'%s' does not exist, is $LTPROOT set correctly?", + bin_dir); + else + SAFE_ASPRINTF(&new_path, "%s:%s", new_path, bin_dir); + } SAFE_SETENV("PATH", new_path, 1); free(new_path);
when LTPROOT set. Put it as the last. TWARN when directory does not exist. Signed-off-by: Petr Vorel <pvorel@suse.cz> --- lib/tst_test.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)