* [LTP] [PATCH 0/3] tools/sparse: Add static check
@ 2021-11-23 12:43 Richard Palethorpe via ltp
2021-11-23 12:43 ` [LTP] [PATCH 1/3] " Richard Palethorpe via ltp
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Richard Palethorpe via ltp @ 2021-11-23 12:43 UTC (permalink / raw)
To: ltp; +Cc: Richard Palethorpe
Hello,
This teaches 'make check' to flag missing static keywords. Note that I
am not sure how this avoids flagging symbols only declared in header
files. However I haven't noticed any false positives and I think false
negatives will only occur when the author uses function prototypes to
reference private symbols before their definition.
The statx patch is just included to show one issue it caught.
Richard Palethorpe (3):
tools/sparse: Add static check
doc: Add LTP-003 and LTP-004 static and tst API prefix rules
statx: Add missing static keyword to tcase variable
doc/library-api-writing-guidelines.txt | 10 +++++
doc/rules.tsv | 2 +
doc/test-writing-guidelines.txt | 8 ++++
testcases/kernel/syscalls/statx/statx01.c | 2 +-
testcases/kernel/syscalls/statx/statx02.c | 2 +-
testcases/kernel/syscalls/statx/statx04.c | 2 +-
testcases/kernel/syscalls/statx/statx05.c | 2 +-
testcases/kernel/syscalls/statx/statx07.c | 2 +-
tools/sparse/sparse-ltp.c | 53 +++++++++++++++++++++++
9 files changed, 78 insertions(+), 5 deletions(-)
--
2.33.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 19+ messages in thread* [LTP] [PATCH 1/3] tools/sparse: Add static check 2021-11-23 12:43 [LTP] [PATCH 0/3] tools/sparse: Add static check Richard Palethorpe via ltp @ 2021-11-23 12:43 ` Richard Palethorpe via ltp 2021-11-24 7:12 ` Petr Vorel ` (2 more replies) 2021-11-23 12:43 ` [LTP] [PATCH 2/3] doc: Add LTP-003 and LTP-004 static and tst API prefix rules Richard Palethorpe via ltp 2021-11-23 12:43 ` [LTP] [PATCH 3/3] statx: Add missing static keyword to tcase variable Richard Palethorpe via ltp 2 siblings, 3 replies; 19+ messages in thread From: Richard Palethorpe via ltp @ 2021-11-23 12:43 UTC (permalink / raw) To: ltp; +Cc: Richard Palethorpe This was adapted from Sparse's inbuilt check_duplicates (-Wdecl). The original check appears to print a warning whenever a symbol is non-static, but has no prototype. It appears to work because library symbols are usually declared first in a header file and then again with their definition in a source file. The LTP version also checks for the various library prefixes, but should otherwise be the same. Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> --- tools/sparse/sparse-ltp.c | 53 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c index 45874e8eb..73725d191 100644 --- a/tools/sparse/sparse-ltp.c +++ b/tools/sparse/sparse-ltp.c @@ -82,6 +82,57 @@ static void do_entrypoint_checks(struct entrypoint *ep) } END_FOR_EACH_PTR(bb); } +/* Check for LTP-003 and LTP-004 + * + * Try to find cases where the static keyword was forgotten. + */ +static void check_symbol_visibility(struct symbol *sym) +{ + struct symbol *next = sym; + unsigned long mod = sym->ctype.modifiers; + const char *name = show_ident(sym->ident); + int has_lib_prefix = !strncmp("tst_", name, 4) || + !strncmp("TST_", name, 4) || + !strncmp("ltp_", name, 4) || + !strncmp("safe_", name, 5); + + if (!(mod & MOD_TOPLEVEL)) + return; + + if (has_lib_prefix && (mod & MOD_STATIC)) { + warning(sym->pos, + "LTP-003: Symbol '%s' has the LTP public library prefix, but is static (private).", + name); + return; + } + + if ((mod & MOD_STATIC)) + return; + + if (tu_kind == LTP_LIB && !has_lib_prefix) { + warning(sym->pos, + "LTP-003: Symbol '%s' is a public library function, but is missing the 'tst_' prefix", + name); + return; + } + + if (next->same_symbol) + return; + + if (sym->ident == &main_ident) + return; + + warning(sym->pos, + "Symbol '%s' has no prototype or library ('tst_') prefix. Should it be static?", + name); +} + +/* AST level checks */ +static void do_symbol_checks(struct symbol *sym) +{ + check_symbol_visibility(sym); +} + /* Compile the AST into a graph of basicblocks */ static void process_symbols(struct symbol_list *list) { @@ -90,6 +141,8 @@ static void process_symbols(struct symbol_list *list) FOR_EACH_PTR(list, sym) { struct entrypoint *ep; + do_symbol_checks(sym); + expand_symbol(sym); ep = linearize_symbol(sym); if (!ep || !ep->entry) -- 2.33.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 1/3] tools/sparse: Add static check 2021-11-23 12:43 ` [LTP] [PATCH 1/3] " Richard Palethorpe via ltp @ 2021-11-24 7:12 ` Petr Vorel 2021-11-30 8:24 ` Richard Palethorpe 2021-11-24 10:00 ` Richard Palethorpe 2021-11-29 10:16 ` Cyril Hrubis 2 siblings, 1 reply; 19+ messages in thread From: Petr Vorel @ 2021-11-24 7:12 UTC (permalink / raw) To: Richard Palethorpe; +Cc: ltp Hi Richie, > This was adapted from Sparse's inbuilt check_duplicates (-Wdecl). The > original check appears to print a warning whenever a symbol is > non-static, but has no prototype. It appears to work because library > symbols are usually declared first in a header file and then again > with their definition in a source file. > The LTP version also checks for the various library prefixes, but > should otherwise be the same. LGTM, thanks! Kind regards, Petr -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 1/3] tools/sparse: Add static check 2021-11-24 7:12 ` Petr Vorel @ 2021-11-30 8:24 ` Richard Palethorpe 2021-11-30 10:19 ` Cyril Hrubis 0 siblings, 1 reply; 19+ messages in thread From: Richard Palethorpe @ 2021-11-30 8:24 UTC (permalink / raw) To: Petr Vorel; +Cc: chrubis, ltp Hello Petr and Cyril, Thanks! pushed. Please pull and try it out. Petr Vorel <pvorel@suse.cz> writes: > Hi Richie, > >> This was adapted from Sparse's inbuilt check_duplicates (-Wdecl). The >> original check appears to print a warning whenever a symbol is >> non-static, but has no prototype. It appears to work because library >> symbols are usually declared first in a header file and then again >> with their definition in a source file. > >> The LTP version also checks for the various library prefixes, but >> should otherwise be the same. > LGTM, thanks! I took the liberty (;-)) of interpreting this as Acked-by. > > Kind regards, > Petr -- Thank you, Richard. -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 1/3] tools/sparse: Add static check 2021-11-30 8:24 ` Richard Palethorpe @ 2021-11-30 10:19 ` Cyril Hrubis 2021-11-30 10:23 ` Cyril Hrubis 0 siblings, 1 reply; 19+ messages in thread From: Cyril Hrubis @ 2021-11-30 10:19 UTC (permalink / raw) To: Richard Palethorpe; +Cc: chrubis, ltp Hi! > Thanks! pushed. Please pull and try it out. Looks like it fails on fuzzy sync since it uses tst_ but it's in an header. These definitions should be static inline and changing them so fixes the warnings. It looks like static inline functions does not make it into the symbol test at all. -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 1/3] tools/sparse: Add static check 2021-11-30 10:19 ` Cyril Hrubis @ 2021-11-30 10:23 ` Cyril Hrubis 2021-11-30 10:33 ` Richard Palethorpe 0 siblings, 1 reply; 19+ messages in thread From: Cyril Hrubis @ 2021-11-30 10:23 UTC (permalink / raw) To: Richard Palethorpe; +Cc: chrubis, ltp Hi! > > Thanks! pushed. Please pull and try it out. > > Looks like it fails on fuzzy sync since it uses tst_ but it's in an > header. > > These definitions should be static inline and changing them so fixes the > warnings. It looks like static inline functions does not make it into > the symbol test at all. This is even stranger, the 'static inline void' functions does not make it into the check function, but anything that returns a non-void value does get there so we need: diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c index 2f32bfa38..b1677d336 100644 --- a/tools/sparse/sparse-ltp.c +++ b/tools/sparse/sparse-ltp.c @@ -98,7 +98,7 @@ static void check_symbol_visibility(const struct symbol *const sym) if (!(mod & MOD_TOPLEVEL)) return; - if (has_lib_prefix && (mod & MOD_STATIC)) { + if (has_lib_prefix && (mod & MOD_STATIC) && !(mod & MOD_INLINE)) { warning(sym->pos, "LTP-003: Symbol '%s' has the LTP public library prefix, but is static (private).", name); And: diff --git a/include/tst_fuzzy_sync.h b/include/tst_fuzzy_sync.h index 8f97bb8f6..bc3450294 100644 --- a/include/tst_fuzzy_sync.h +++ b/include/tst_fuzzy_sync.h @@ -210,7 +210,7 @@ struct tst_fzsync_pair { * * @sa tst_fzsync_pair_reset() */ -static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) +static inline void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) { CHK(avg_alpha, 0, 1, 0.25); CHK(min_samples, 20, INT_MAX, 1024); @@ -230,7 +230,7 @@ static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) * * Call this from your cleanup function. */ -static void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) +static inline void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) { if (pair->thread_b) { /* Revoke thread B if parent hits accidental break */ @@ -254,7 +254,7 @@ struct tst_fzsync_run_thread { * Wrap run_b for tst_fzsync_pair_reset to enable pthread cancel * at the start of the thread B. */ -static void *tst_fzsync_thread_wrapper(void *run_thread) +static inline void *tst_fzsync_thread_wrapper(void *run_thread) { struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread *)run_thread; @@ -268,7 +268,7 @@ static void *tst_fzsync_thread_wrapper(void *run_thread) * * @relates tst_fzsync_stat */ -static void tst_init_stat(struct tst_fzsync_stat *s) +static inline void tst_init_stat(struct tst_fzsync_stat *s) { s->avg = 0; s->avg_dev = 0; @@ -292,7 +292,7 @@ static void tst_init_stat(struct tst_fzsync_stat *s) * * @sa tst_fzsync_pair_init() */ -static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, +static inline void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, void *(*run_b)(void *)) { tst_fzsync_pair_cleanup(pair); @@ -340,7 +340,7 @@ static inline void tst_fzsync_stat_info(struct tst_fzsync_stat stat, * * @relates tst_fzsync_pair */ -static void tst_fzsync_pair_info(struct tst_fzsync_pair *pair) +static inline void tst_fzsync_pair_info(struct tst_fzsync_pair *pair) { tst_res(TINFO, "loop = %d, delay_bias = %d", pair->exec_loop, pair->delay_bias); @@ -493,7 +493,7 @@ static inline void tst_upd_diff_stat(struct tst_fzsync_stat *s, * * @relates tst_fzsync_pair */ -static void tst_fzsync_pair_update(struct tst_fzsync_pair *pair) +static inline void tst_fzsync_pair_update(struct tst_fzsync_pair *pair) { float alpha = pair->avg_alpha; float per_spin_time, time_delay; -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 1/3] tools/sparse: Add static check 2021-11-30 10:23 ` Cyril Hrubis @ 2021-11-30 10:33 ` Richard Palethorpe 2021-11-30 10:51 ` Cyril Hrubis 2021-11-30 11:43 ` Cyril Hrubis 0 siblings, 2 replies; 19+ messages in thread From: Richard Palethorpe @ 2021-11-30 10:33 UTC (permalink / raw) To: Cyril Hrubis; +Cc: chrubis, ltp Hello, Cyril Hrubis <chrubis@suse.cz> writes: > Hi! >> > Thanks! pushed. Please pull and try it out. >> >> Looks like it fails on fuzzy sync since it uses tst_ but it's in an >> header. >> >> These definitions should be static inline and changing them so fixes the >> warnings. It looks like static inline functions does not make it into >> the symbol test at all. Ah, I vaguely remember now that "static inline" is the correct way to include functions in header files. Otherwise they shouldn't be in header files. We have LTO now as well so possibly Fuzzy sync shouldn't be all in the header, but that's another matter. > > This is even stranger, the 'static inline void' functions does not make > it into the check function, but anything that returns a non-void value > does get there so we need: I have no idea... > > diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c > index 2f32bfa38..b1677d336 100644 > --- a/tools/sparse/sparse-ltp.c > +++ b/tools/sparse/sparse-ltp.c > @@ -98,7 +98,7 @@ static void check_symbol_visibility(const struct symbol *const sym) > if (!(mod & MOD_TOPLEVEL)) > return; > > - if (has_lib_prefix && (mod & MOD_STATIC)) { > + if (has_lib_prefix && (mod & MOD_STATIC) && !(mod & MOD_INLINE)) { > warning(sym->pos, > "LTP-003: Symbol '%s' has the LTP public library prefix, but is static (private).", > name); > OK, this loooks good and below. Could you send a patch? Also can add Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com> > And: > > diff --git a/include/tst_fuzzy_sync.h b/include/tst_fuzzy_sync.h > index 8f97bb8f6..bc3450294 100644 > --- a/include/tst_fuzzy_sync.h > +++ b/include/tst_fuzzy_sync.h > @@ -210,7 +210,7 @@ struct tst_fzsync_pair { > * > * @sa tst_fzsync_pair_reset() > */ > -static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) > +static inline void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) > { > CHK(avg_alpha, 0, 1, 0.25); > CHK(min_samples, 20, INT_MAX, 1024); > @@ -230,7 +230,7 @@ static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) > * > * Call this from your cleanup function. > */ > -static void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) > +static inline void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) > { > if (pair->thread_b) { > /* Revoke thread B if parent hits accidental break */ > @@ -254,7 +254,7 @@ struct tst_fzsync_run_thread { > * Wrap run_b for tst_fzsync_pair_reset to enable pthread cancel > * at the start of the thread B. > */ > -static void *tst_fzsync_thread_wrapper(void *run_thread) > +static inline void *tst_fzsync_thread_wrapper(void *run_thread) > { > struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread *)run_thread; > > @@ -268,7 +268,7 @@ static void *tst_fzsync_thread_wrapper(void *run_thread) > * > * @relates tst_fzsync_stat > */ > -static void tst_init_stat(struct tst_fzsync_stat *s) > +static inline void tst_init_stat(struct tst_fzsync_stat *s) > { > s->avg = 0; > s->avg_dev = 0; > @@ -292,7 +292,7 @@ static void tst_init_stat(struct tst_fzsync_stat *s) > * > * @sa tst_fzsync_pair_init() > */ > -static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, > +static inline void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, > void *(*run_b)(void *)) > { > tst_fzsync_pair_cleanup(pair); > @@ -340,7 +340,7 @@ static inline void tst_fzsync_stat_info(struct tst_fzsync_stat stat, > * > * @relates tst_fzsync_pair > */ > -static void tst_fzsync_pair_info(struct tst_fzsync_pair *pair) > +static inline void tst_fzsync_pair_info(struct tst_fzsync_pair *pair) > { > tst_res(TINFO, "loop = %d, delay_bias = %d", > pair->exec_loop, pair->delay_bias); > @@ -493,7 +493,7 @@ static inline void tst_upd_diff_stat(struct tst_fzsync_stat *s, > * > * @relates tst_fzsync_pair > */ > -static void tst_fzsync_pair_update(struct tst_fzsync_pair *pair) > +static inline void tst_fzsync_pair_update(struct tst_fzsync_pair *pair) > { > float alpha = pair->avg_alpha; > float per_spin_time, time_delay; -- Thank you, Richard. -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 1/3] tools/sparse: Add static check 2021-11-30 10:33 ` Richard Palethorpe @ 2021-11-30 10:51 ` Cyril Hrubis 2021-11-30 11:43 ` Cyril Hrubis 1 sibling, 0 replies; 19+ messages in thread From: Cyril Hrubis @ 2021-11-30 10:51 UTC (permalink / raw) To: Richard Palethorpe; +Cc: chrubis, ltp Hi! > >> > Thanks! pushed. Please pull and try it out. > >> > >> Looks like it fails on fuzzy sync since it uses tst_ but it's in an > >> header. > >> > >> These definitions should be static inline and changing them so fixes the > >> warnings. It looks like static inline functions does not make it into > >> the symbol test at all. > > Ah, I vaguely remember now that "static inline" is the correct way to > include functions in header files. Otherwise they shouldn't be in header > files. We have LTO now as well so possibly Fuzzy sync shouldn't be all > in the header, but that's another matter. If I recall correctly LTO is currently broken on ARM and even if that was working we still have to support 10 years old compilers anyways... -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 1/3] tools/sparse: Add static check 2021-11-30 10:33 ` Richard Palethorpe 2021-11-30 10:51 ` Cyril Hrubis @ 2021-11-30 11:43 ` Cyril Hrubis 1 sibling, 0 replies; 19+ messages in thread From: Cyril Hrubis @ 2021-11-30 11:43 UTC (permalink / raw) To: Richard Palethorpe; +Cc: chrubis, ltp Hi! I guess that we can also silence some of the useless output by skipping some old library crap as: diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c index b1677d336..1a3b4089a 100644 --- a/tools/sparse/sparse-ltp.c +++ b/tools/sparse/sparse-ltp.c @@ -98,6 +98,9 @@ static void check_symbol_visibility(const struct symbol *const sym) if (!(mod & MOD_TOPLEVEL)) return; + if (!strcmp(name, "TCID") || !strcmp(name, "TST_TOTAL")) + return; + if (has_lib_prefix && (mod & MOD_STATIC) && !(mod & MOD_INLINE)) { warning(sym->pos, "LTP-003: Symbol '%s' has the LTP public library prefix, but is static (private).", Will send another patch. -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 1/3] tools/sparse: Add static check 2021-11-23 12:43 ` [LTP] [PATCH 1/3] " Richard Palethorpe via ltp 2021-11-24 7:12 ` Petr Vorel @ 2021-11-24 10:00 ` Richard Palethorpe 2021-11-29 10:16 ` Cyril Hrubis 2 siblings, 0 replies; 19+ messages in thread From: Richard Palethorpe @ 2021-11-24 10:00 UTC (permalink / raw) To: ltp; +Cc: Richard Palethorpe Hello, Richard Palethorpe <rpalethorpe@suse.com> writes: > This was adapted from Sparse's inbuilt check_duplicates (-Wdecl). The > original check appears to print a warning whenever a symbol is > non-static, but has no prototype. It appears to work because library > symbols are usually declared first in a header file and then again > with their definition in a source file. > > The LTP version also checks for the various library prefixes, but > should otherwise be the same. > > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> > --- > tools/sparse/sparse-ltp.c | 53 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c > index 45874e8eb..73725d191 100644 > --- a/tools/sparse/sparse-ltp.c > +++ b/tools/sparse/sparse-ltp.c > @@ -82,6 +82,57 @@ static void do_entrypoint_checks(struct entrypoint *ep) > } END_FOR_EACH_PTR(bb); > } > > +/* Check for LTP-003 and LTP-004 > + * > + * Try to find cases where the static keyword was forgotten. > + */ > +static void check_symbol_visibility(struct symbol *sym) > +{ > + struct symbol *next = sym; Ooops, we don't need next anymore. -- Thank you, Richard. -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 1/3] tools/sparse: Add static check 2021-11-23 12:43 ` [LTP] [PATCH 1/3] " Richard Palethorpe via ltp 2021-11-24 7:12 ` Petr Vorel 2021-11-24 10:00 ` Richard Palethorpe @ 2021-11-29 10:16 ` Cyril Hrubis 2 siblings, 0 replies; 19+ messages in thread From: Cyril Hrubis @ 2021-11-29 10:16 UTC (permalink / raw) To: Richard Palethorpe; +Cc: ltp Hi! Looks good to me as well. Acked-by: Cyril Hrubis <chrubis@suse.cz> -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* [LTP] [PATCH 2/3] doc: Add LTP-003 and LTP-004 static and tst API prefix rules 2021-11-23 12:43 [LTP] [PATCH 0/3] tools/sparse: Add static check Richard Palethorpe via ltp 2021-11-23 12:43 ` [LTP] [PATCH 1/3] " Richard Palethorpe via ltp @ 2021-11-23 12:43 ` Richard Palethorpe via ltp 2021-11-24 7:18 ` Petr Vorel 2021-11-29 10:44 ` Cyril Hrubis 2021-11-23 12:43 ` [LTP] [PATCH 3/3] statx: Add missing static keyword to tcase variable Richard Palethorpe via ltp 2 siblings, 2 replies; 19+ messages in thread From: Richard Palethorpe via ltp @ 2021-11-23 12:43 UTC (permalink / raw) To: ltp; +Cc: Richard Palethorpe Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> --- doc/library-api-writing-guidelines.txt | 10 ++++++++++ doc/rules.tsv | 2 ++ doc/test-writing-guidelines.txt | 8 ++++++++ 3 files changed, 20 insertions(+) diff --git a/doc/library-api-writing-guidelines.txt b/doc/library-api-writing-guidelines.txt index 2819d4177..c82053681 100644 --- a/doc/library-api-writing-guidelines.txt +++ b/doc/library-api-writing-guidelines.txt @@ -39,6 +39,16 @@ The macros which are clearly intended to update these variables. That is +TEST+ and those in 'tst_test_macros.h'. Are of course allowed to update these variables. +2.3 LTP-003: Externally visible library symbols have the tst_ prefix +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Functions, types and variables in the public test API should have the +tst_ prefix. With some exceptions for symbols already prefixed with +safe_ or ltp_. + +Static (private) symbols should not have the prefix. + + 3. Shell API ------------ diff --git a/doc/rules.tsv b/doc/rules.tsv index d4081ce0f..2dfeca9f9 100644 --- a/doc/rules.tsv +++ b/doc/rules.tsv @@ -1,3 +1,5 @@ ID DESCRIPTION LTP-001 Library source files have tst_ prefix LTP-002 TST_RET and TST_ERR are never modified by test library functions +LTP-003 Externally visible library symbols have the tst_ prefix +LTP-004 Test executable symbols are marked static diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt index b87446d1b..98fdb4d8d 100644 --- a/doc/test-writing-guidelines.txt +++ b/doc/test-writing-guidelines.txt @@ -133,6 +133,14 @@ script from kernel git tree. NOTE: If `make check` does not report any problems, the code still may be wrong as all tools used for checking only look for common mistakes. +2.1.1 LTP-004: Test executable symbols are marked static +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Test executables should not export symbols unecessarily. This means +that all top-level variables and functions should be marked with the +static keyword. The only visible symbols should be those included from +shared object files. + 2.2 Shell coding style ^^^^^^^^^^^^^^^^^^^^^^ -- 2.33.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 2/3] doc: Add LTP-003 and LTP-004 static and tst API prefix rules 2021-11-23 12:43 ` [LTP] [PATCH 2/3] doc: Add LTP-003 and LTP-004 static and tst API prefix rules Richard Palethorpe via ltp @ 2021-11-24 7:18 ` Petr Vorel 2021-11-29 10:17 ` Cyril Hrubis 2021-11-29 10:44 ` Cyril Hrubis 1 sibling, 1 reply; 19+ messages in thread From: Petr Vorel @ 2021-11-24 7:18 UTC (permalink / raw) To: Richard Palethorpe; +Cc: ltp Hi Richie, ... > +2.3 LTP-003: Externally visible library symbols have the tst_ prefix > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +Functions, types and variables in the public test API should have the > +tst_ prefix. With some exceptions for symbols already prefixed with > +safe_ or ltp_. BTW It'd be nice to have some check for shell library (maybe shellcheck would be able to do it). ... > +2.1.1 LTP-004: Test executable symbols are marked static > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Test executables should not export symbols unecessarily. This means typo s/unecessarily/unnecessarily/ > +that all top-level variables and functions should be marked with the > +static keyword. The only visible symbols should be those included from > +shared object files. > + Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 2/3] doc: Add LTP-003 and LTP-004 static and tst API prefix rules 2021-11-24 7:18 ` Petr Vorel @ 2021-11-29 10:17 ` Cyril Hrubis 2021-11-29 10:33 ` Petr Vorel 0 siblings, 1 reply; 19+ messages in thread From: Cyril Hrubis @ 2021-11-29 10:17 UTC (permalink / raw) To: Petr Vorel; +Cc: Richard Palethorpe, ltp Hi! > BTW It'd be nice to have some check for shell library (maybe shellcheck would be > able to do it). Actually the check is in the shell library script. -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 2/3] doc: Add LTP-003 and LTP-004 static and tst API prefix rules 2021-11-29 10:17 ` Cyril Hrubis @ 2021-11-29 10:33 ` Petr Vorel 0 siblings, 0 replies; 19+ messages in thread From: Petr Vorel @ 2021-11-29 10:33 UTC (permalink / raw) To: Cyril Hrubis; +Cc: Richard Palethorpe, ltp > Hi! > > BTW It'd be nice to have some check for shell library (maybe shellcheck would be > > able to do it). > Actually the check is in the shell library script. I'm avare only about the check in tst_run() which check that tests are not using variables starting with TST or _tst_ prefix, which is important. My remark was about "LTP-003: Externally visible library symbols have the tst_ prefix", which is slightly different think. Not sure if can be scripted, thus feel free to ignore it. I have more remarks on shell checking, but post it as a separate thread. Kind regards, Petr -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 2/3] doc: Add LTP-003 and LTP-004 static and tst API prefix rules 2021-11-23 12:43 ` [LTP] [PATCH 2/3] doc: Add LTP-003 and LTP-004 static and tst API prefix rules Richard Palethorpe via ltp 2021-11-24 7:18 ` Petr Vorel @ 2021-11-29 10:44 ` Cyril Hrubis 1 sibling, 0 replies; 19+ messages in thread From: Cyril Hrubis @ 2021-11-29 10:44 UTC (permalink / raw) To: Richard Palethorpe; +Cc: ltp Hi! Withe the typos fixed: Reviewed-by: Cyril Hrubis <chrubis@suse.cz> -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* [LTP] [PATCH 3/3] statx: Add missing static keyword to tcase variable 2021-11-23 12:43 [LTP] [PATCH 0/3] tools/sparse: Add static check Richard Palethorpe via ltp 2021-11-23 12:43 ` [LTP] [PATCH 1/3] " Richard Palethorpe via ltp 2021-11-23 12:43 ` [LTP] [PATCH 2/3] doc: Add LTP-003 and LTP-004 static and tst API prefix rules Richard Palethorpe via ltp @ 2021-11-23 12:43 ` Richard Palethorpe via ltp 2021-11-24 7:19 ` Petr Vorel 2 siblings, 1 reply; 19+ messages in thread From: Richard Palethorpe via ltp @ 2021-11-23 12:43 UTC (permalink / raw) To: ltp; +Cc: Richard Palethorpe This shows how easy it is to miss the static keyword. Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> --- testcases/kernel/syscalls/statx/statx01.c | 2 +- testcases/kernel/syscalls/statx/statx02.c | 2 +- testcases/kernel/syscalls/statx/statx04.c | 2 +- testcases/kernel/syscalls/statx/statx05.c | 2 +- testcases/kernel/syscalls/statx/statx07.c | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/testcases/kernel/syscalls/statx/statx01.c b/testcases/kernel/syscalls/statx/statx01.c index 2358dd7bc..11e188e8f 100644 --- a/testcases/kernel/syscalls/statx/statx01.c +++ b/testcases/kernel/syscalls/statx/statx01.c @@ -131,7 +131,7 @@ static void test_device_file(void) } -struct tcase { +static struct tcase { void (*tfunc)(void); } tcases[] = { {&test_normal_file}, diff --git a/testcases/kernel/syscalls/statx/statx02.c b/testcases/kernel/syscalls/statx/statx02.c index 08ea940cb..15f5562b2 100644 --- a/testcases/kernel/syscalls/statx/statx02.c +++ b/testcases/kernel/syscalls/statx/statx02.c @@ -90,7 +90,7 @@ static void test_sym_link(void) "Statx symlink flag failed to work as expected"); } -struct tcase { +static struct tcase { void (*tfunc)(void); } tcases[] = { {&test_empty_path}, diff --git a/testcases/kernel/syscalls/statx/statx04.c b/testcases/kernel/syscalls/statx/statx04.c index 180c61bf9..f66b04f70 100644 --- a/testcases/kernel/syscalls/statx/statx04.c +++ b/testcases/kernel/syscalls/statx/statx04.c @@ -133,7 +133,7 @@ static void test_unflagged(void) tst_res(TFAIL, "STATX_ATTR_NODUMP flag is set"); } -struct test_cases { +static struct test_cases { void (*tfunc)(void); } tcases[] = { {&test_flagged}, diff --git a/testcases/kernel/syscalls/statx/statx05.c b/testcases/kernel/syscalls/statx/statx05.c index 81a5bcbf2..47a1f8ad1 100644 --- a/testcases/kernel/syscalls/statx/statx05.c +++ b/testcases/kernel/syscalls/statx/statx05.c @@ -74,7 +74,7 @@ static void test_unflagged(void) tst_res(TFAIL, "STATX_ATTR_ENCRYPTED flag is set"); } -struct test_cases { +static struct test_cases { void (*tfunc)(void); } tcases[] = { {&test_flagged}, diff --git a/testcases/kernel/syscalls/statx/statx07.c b/testcases/kernel/syscalls/statx/statx07.c index ec1cdd190..9aa160d31 100644 --- a/testcases/kernel/syscalls/statx/statx07.c +++ b/testcases/kernel/syscalls/statx/statx07.c @@ -84,7 +84,7 @@ static int get_mode(char *file_name, int flag_type, char *flag_name) return buf.stx_mode; } -const struct test_cases { +static const struct test_cases { int flag; char *flag_name; char *server_file; -- 2.33.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 3/3] statx: Add missing static keyword to tcase variable 2021-11-23 12:43 ` [LTP] [PATCH 3/3] statx: Add missing static keyword to tcase variable Richard Palethorpe via ltp @ 2021-11-24 7:19 ` Petr Vorel 2021-11-29 10:16 ` Cyril Hrubis 0 siblings, 1 reply; 19+ messages in thread From: Petr Vorel @ 2021-11-24 7:19 UTC (permalink / raw) To: Richard Palethorpe; +Cc: ltp Hi Richie, obviously correct. Thanks! Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [LTP] [PATCH 3/3] statx: Add missing static keyword to tcase variable 2021-11-24 7:19 ` Petr Vorel @ 2021-11-29 10:16 ` Cyril Hrubis 0 siblings, 0 replies; 19+ messages in thread From: Cyril Hrubis @ 2021-11-29 10:16 UTC (permalink / raw) To: Petr Vorel; +Cc: Richard Palethorpe, ltp Hi! > obviously correct. Thanks! > > Reviewed-by: Petr Vorel <pvorel@suse.cz> Second to this. Reviewed-by: Cyril Hrubis <chrubis@suse.cz> -- Cyril Hrubis chrubis@suse.cz -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-11-30 11:43 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-23 12:43 [LTP] [PATCH 0/3] tools/sparse: Add static check Richard Palethorpe via ltp 2021-11-23 12:43 ` [LTP] [PATCH 1/3] " Richard Palethorpe via ltp 2021-11-24 7:12 ` Petr Vorel 2021-11-30 8:24 ` Richard Palethorpe 2021-11-30 10:19 ` Cyril Hrubis 2021-11-30 10:23 ` Cyril Hrubis 2021-11-30 10:33 ` Richard Palethorpe 2021-11-30 10:51 ` Cyril Hrubis 2021-11-30 11:43 ` Cyril Hrubis 2021-11-24 10:00 ` Richard Palethorpe 2021-11-29 10:16 ` Cyril Hrubis 2021-11-23 12:43 ` [LTP] [PATCH 2/3] doc: Add LTP-003 and LTP-004 static and tst API prefix rules Richard Palethorpe via ltp 2021-11-24 7:18 ` Petr Vorel 2021-11-29 10:17 ` Cyril Hrubis 2021-11-29 10:33 ` Petr Vorel 2021-11-29 10:44 ` Cyril Hrubis 2021-11-23 12:43 ` [LTP] [PATCH 3/3] statx: Add missing static keyword to tcase variable Richard Palethorpe via ltp 2021-11-24 7:19 ` Petr Vorel 2021-11-29 10:16 ` Cyril Hrubis
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.