| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8dcb048f-ed24-f75d-2782-13eacb64c807@linuxfoundation.org>
Date: Mon, 25 Jan 2021 19:08:55 -0700
From: Shuah Khan <skhan@...uxfoundation.org>
To: Fenghua Yu <fenghua.yu@...el.com>, Shuah Khan <shuah@...nel.org>,
Tony Luck <tony.luck@...el.com>,
Reinette Chatre <reinette.chatre@...el.com>,
David Binderman <dcb314@...mail.com>,
Babu Moger <babu.moger@....com>,
James Morse <james.morse@....com>,
Ravi V Shankar <ravi.v.shankar@...el.com>,
Shuah Khan <skhan@...uxfoundation.org>
Cc: linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 02/17] selftests/resctrl: Clean up resctrl features
check
On 11/30/20 1:19 PM, Fenghua Yu wrote:
> Checking resctrl features call strcmp() to compare feature strings
> (e.g. "mba", "cat" etc). The checkings are error prone and don't have
> good coding style. Define the constant strings in macros and call
> strncmp() to solve the potential issues.
>
> Suggested-by: Shuah Khan <shuah@...nel.org>
> Signed-off-by: Fenghua Yu <fenghua.yu@...el.com>
> ---
> tools/testing/selftests/resctrl/cache.c | 8 +++---
> tools/testing/selftests/resctrl/cat_test.c | 2 +-
> tools/testing/selftests/resctrl/cqm_test.c | 2 +-
> tools/testing/selftests/resctrl/fill_buf.c | 4 +--
> tools/testing/selftests/resctrl/mba_test.c | 2 +-
> tools/testing/selftests/resctrl/mbm_test.c | 2 +-
> tools/testing/selftests/resctrl/resctrl.h | 25 +++++++++++++++++++
> .../testing/selftests/resctrl/resctrl_tests.c | 12 ++++-----
> tools/testing/selftests/resctrl/resctrl_val.c | 19 ++++++--------
> tools/testing/selftests/resctrl/resctrlfs.c | 14 +++++------
> 10 files changed, 55 insertions(+), 35 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 38dbf4962e33..248bf000c978 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -182,7 +182,7 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
> /*
> * Measure cache miss from perf.
> */
> - if (!strcmp(param->resctrl_val, "cat")) {
> + if (is_cat(param->resctrl_val)) {
Is there reason to add this function for one line of code?
Same comment for all these is_* routines in this patch.
> ret = get_llc_perf(&llc_perf_miss);
> if (ret < 0)
> return ret;
> @@ -192,7 +192,7 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
> /*
> * Measure llc occupancy from resctrl.
> */
> - if (!strcmp(param->resctrl_val, "cqm")) {
> + if (is_cqm(param->resctrl_val)) {
> ret = get_llc_occu_resctrl(&llc_occu_resc);
> if (ret < 0)
> return ret;
> @@ -234,7 +234,7 @@ int cat_val(struct resctrl_val_param *param)
> if (ret)
> return ret;
>
> - if ((strcmp(resctrl_val, "cat") == 0)) {
> + if (is_cat(resctrl_val)) {
> ret = initialize_llc_perf();
> if (ret)
> return ret;
> @@ -242,7 +242,7 @@ int cat_val(struct resctrl_val_param *param)
>
> /* Test runs until the callback setup() tells the test to stop. */
> while (1) {
> - if (strcmp(resctrl_val, "cat") == 0) {
> + if (is_cat(resctrl_val)) {
> ret = param->setup(1, param);
> if (ret) {
> ret = 0;
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 7f723bd8f328..6d9a41f3939a 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -160,7 +160,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
> return -1;
>
> struct resctrl_val_param param = {
> - .resctrl_val = "cat",
> + .resctrl_val = CAT_STR,
> .cpu_no = cpu_no,
> .mum_resctrlfs = 0,
> .setup = cat_setup,
> diff --git a/tools/testing/selftests/resctrl/cqm_test.c b/tools/testing/selftests/resctrl/cqm_test.c
> index b6af940ccfc2..6635b24a74cc 100644
> --- a/tools/testing/selftests/resctrl/cqm_test.c
> +++ b/tools/testing/selftests/resctrl/cqm_test.c
> @@ -142,7 +142,7 @@ int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
> }
>
> struct resctrl_val_param param = {
> - .resctrl_val = "cqm",
> + .resctrl_val = CQM_STR,
> .ctrlgrp = "c1",
> .mongrp = "m1",
> .cpu_no = cpu_no,
> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> index 79c611c99a3d..bece8bb4b575 100644
> --- a/tools/testing/selftests/resctrl/fill_buf.c
> +++ b/tools/testing/selftests/resctrl/fill_buf.c
> @@ -115,7 +115,7 @@ static int fill_cache_read(unsigned char *start_ptr, unsigned char *end_ptr,
>
> while (1) {
> ret = fill_one_span_read(start_ptr, end_ptr);
> - if (!strcmp(resctrl_val, "cat"))
> + if (is_cat(resctrl_val))
> break;
> }
>
> @@ -134,7 +134,7 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
> {
> while (1) {
> fill_one_span_write(start_ptr, end_ptr);
> - if (!strcmp(resctrl_val, "cat"))
> + if (is_cat(resctrl_val))
> break;
> }
>
> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index 7bf8eaa6204b..6449fbd96096 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -141,7 +141,7 @@ void mba_test_cleanup(void)
> int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
> {
> struct resctrl_val_param param = {
> - .resctrl_val = "mba",
> + .resctrl_val = MBA_STR,
> .ctrlgrp = "c1",
> .mongrp = "m1",
> .cpu_no = cpu_no,
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 4700f7453f81..ec6cfe01c9c2 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -114,7 +114,7 @@ void mbm_test_cleanup(void)
> int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd)
> {
> struct resctrl_val_param param = {
> - .resctrl_val = "mbm",
> + .resctrl_val = MBM_STR,
> .ctrlgrp = "c1",
> .mongrp = "m1",
> .span = span,
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 12b77182cb44..bfbc16b39a9e 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -62,6 +62,31 @@ struct resctrl_val_param {
> int (*setup)(int num, ...);
> };
>
> +#define MBM_STR "mbm"
> +#define MBA_STR "mba"
> +#define CQM_STR "cqm"
> +#define CAT_STR "cat"
> +
This is fine.
> +static inline bool is_mbm(const char *str)
> +{
> + return !strncmp(str, MBM_STR, 3);
Why not use sizeof(MBM_STR) instead of hardcoding?
Same comment on all the other such usahes below.
> +}
> +
There is no need to add an entire routine for this.
> +static inline bool is_mba(const char *str)
> +{
> + return !strncmp(str, MBA_STR, 3);
> +}
> +
There is no need to add an entire routine for this.
> +static inline bool is_cqm(const char *str)
> +{
> + return !strncmp(str, CQM_STR, 3);
> +}
> +
There is no need to add an entire routine for this.
> +static inline bool is_cat(const char *str)
> +{
> + return !strncmp(str, CAT_STR, 3);
> +}
> +
There is no need to add an entire routine for this.
> extern pid_t bm_pid, ppid;
> extern int tests_run;
>
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 425cc85ac883..f425eaf8c331 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -85,13 +85,13 @@ int main(int argc, char **argv)
> cqm_test = false;
> cat_test = false;
> while (token) {
> - if (!strcmp(token, "mbm")) {
> + if (is_mbm(token)) {
> mbm_test = true;
> - } else if (!strcmp(token, "mba")) {
> + } else if (is_mba(token)) {
> mba_test = true;
> - } else if (!strcmp(token, "cqm")) {
> + } else if (is_cqm(token)) {
> cqm_test = true;
> - } else if (!strcmp(token, "cat")) {
> + } else if (is_cat(token)) {
> cat_test = true;
> } else {
> printf("invalid argument\n");
> @@ -161,7 +161,7 @@ int main(int argc, char **argv)
> if (!is_amd && mbm_test) {
> printf("# Starting MBM BW change ...\n");
> if (!has_ben)
> - sprintf(benchmark_cmd[5], "%s", "mba");
> + sprintf(benchmark_cmd[5], "%s", MBA_STR);
> res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd);
> printf("%sok MBM: bw change\n", res ? "not " : "");
> mbm_test_cleanup();
> @@ -181,7 +181,7 @@ int main(int argc, char **argv)
> if (cqm_test) {
> printf("# Starting CQM test ...\n");
> if (!has_ben)
> - sprintf(benchmark_cmd[5], "%s", "cqm");
> + sprintf(benchmark_cmd[5], "%s", CQM_STR);
> res = cqm_resctrl_val(cpu_no, no_of_bits, benchmark_cmd);
> printf("%sok CQM: test\n", res ? "not " : "");
> cqm_test_cleanup();
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 520fea3606d1..f55e04a30a77 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -397,10 +397,10 @@ static void initialize_mem_bw_resctrl(const char *ctrlgrp, const char *mongrp,
> return;
> }
>
> - if (strcmp(resctrl_val, "mbm") == 0)
> + if (is_mbm(resctrl_val))
> set_mbm_path(ctrlgrp, mongrp, resource_id);
>
> - if ((strcmp(resctrl_val, "mba") == 0)) {
> + if (is_mba(resctrl_val)) {
> if (ctrlgrp)
> sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH,
> RESCTRL_PATH, ctrlgrp, resource_id);
> @@ -524,7 +524,7 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
> return;
> }
>
> - if (strcmp(resctrl_val, "cqm") == 0)
> + if (is_cqm(resctrl_val))
> set_cqm_path(ctrlgrp, mongrp, resource_id);
> }
>
> @@ -579,8 +579,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
> if (strcmp(param->filename, "") == 0)
> sprintf(param->filename, "stdio");
>
> - if ((strcmp(resctrl_val, "mba")) == 0 ||
> - (strcmp(resctrl_val, "mbm")) == 0) {
> + if (is_mba(resctrl_val) || is_mbm(resctrl_val)) {
> ret = validate_bw_report_request(param->bw_report);
> if (ret)
> return ret;
> @@ -674,15 +673,14 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
> if (ret)
> goto out;
>
> - if ((strcmp(resctrl_val, "mbm") == 0) ||
> - (strcmp(resctrl_val, "mba") == 0)) {
> + if (is_mbm(resctrl_val) || is_mba(resctrl_val)) {
> ret = initialize_mem_bw_imc();
> if (ret)
> goto out;
>
> initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
> param->cpu_no, resctrl_val);
> - } else if (strcmp(resctrl_val, "cqm") == 0)
> + } else if (is_cqm(resctrl_val))
> initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
> param->cpu_no, resctrl_val);
>
> @@ -710,8 +708,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
>
> /* Test runs until the callback setup() tells the test to stop. */
> while (1) {
> - if ((strcmp(resctrl_val, "mbm") == 0) ||
> - (strcmp(resctrl_val, "mba") == 0)) {
> + if (is_mbm(resctrl_val) || is_mba(resctrl_val)) {
> ret = param->setup(1, param);
> if (ret) {
> ret = 0;
> @@ -721,7 +718,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
> ret = measure_vals(param, &bw_resc_start);
> if (ret)
> break;
> - } else if (strcmp(resctrl_val, "cqm") == 0) {
> + } else if (is_cqm(resctrl_val)) {
> ret = param->setup(1, param);
> if (ret) {
> ret = 0;
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 2a16100c9c3f..dc4f1286aefa 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -334,7 +334,7 @@ void run_benchmark(int signum, siginfo_t *info, void *ucontext)
> operation = atoi(benchmark_cmd[4]);
> sprintf(resctrl_val, "%s", benchmark_cmd[5]);
>
> - if (strcmp(resctrl_val, "cqm") != 0)
> + if (!is_cqm(resctrl_val))
> buffer_span = span * MB;
> else
> buffer_span = span;
> @@ -459,8 +459,7 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
> goto out;
>
> /* Create mon grp and write pid into it for "mbm" and "cqm" test */
> - if ((strcmp(resctrl_val, "cqm") == 0) ||
> - (strcmp(resctrl_val, "mbm") == 0)) {
> + if (is_cqm(resctrl_val) || is_mbm(resctrl_val)) {
> if (strlen(mongrp)) {
> sprintf(monitorgroup_p, "%s/mon_groups", controlgroup);
> sprintf(monitorgroup, "%s/%s", monitorgroup_p, mongrp);
> @@ -505,9 +504,8 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
> int resource_id, ret = 0;
> FILE *fp;
>
> - if ((strcmp(resctrl_val, "mba") != 0) &&
> - (strcmp(resctrl_val, "cat") != 0) &&
> - (strcmp(resctrl_val, "cqm") != 0))
> + if (!is_mba(resctrl_val) && !is_cat(resctrl_val) &&
> + !is_cqm(resctrl_val))
> return -ENOENT;
>
> if (!schemata) {
> @@ -528,9 +526,9 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
> else
> sprintf(controlgroup, "%s/schemata", RESCTRL_PATH);
>
> - if (!strcmp(resctrl_val, "cat") || !strcmp(resctrl_val, "cqm"))
> + if (is_cat(resctrl_val) || is_cqm(resctrl_val))
> sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
> - if (strcmp(resctrl_val, "mba") == 0)
> + if (is_mba(resctrl_val))
> sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
>
> fp = fopen(controlgroup, "w");
>
thanks,
-- Shuah
Powered by blists - more mailing lists