From 44cc78222d51346899436dee01c252bf18ee3e77 Mon Sep 17 00:00:00 2001 From: Mr NeilBrown Date: Tue, 24 Nov 2020 15:53:46 +1100 Subject: [PATCH] LU-9859 ptlrpc: simplifying expression parsing in nrs_tbf The standard approach to parsing in the kernel is to modify strings as needed, such as to nul-terminate substrings. Lustre tends to pass around lengths instead, which means that various kernel functions such as kstrtoNN() or strsep() or even strcmp() cannot be used. We can simplify code in nrs_tbf if we kstrdup() strings before parsing them, and then use standard functions. cfs_gettok() strips spaces while finding the token. With this patch, stripping of spaces is left to the final stage where an expression (a={b}) is being parsed. It might arrive with arbitrary space such as " a ={ b } ". A test in sanityn has some spaces added in various places to ensure that are parsed correctly as an earlier version of this patch got some of that wrong. The list parsed in nrs_tbf_id_list_parse() can have multiple separator (spaces) between elements, which contrasts with expressions which only have a single '=" or "&" etc. So strsep() might return an empty token between two consecutive spaces. This is not necessarily an error - it is only an error if *all* tokens are empty. So we add a "list_empty()" test at the end. Test-Parameters: trivial Test-Parameters: testlist=sanityn Test-Parameters: testlist=sanity Signed-off-by: Mr NeilBrown Change-Id: Id4fb399773e49e4869ca5ebf93fe63c864d82287 Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/50835 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Etienne AUJAMES Reviewed-by: James Simmons Reviewed-by: Oleg Drokin --- lustre/ptlrpc/nrs_tbf.c | 153 +++++++++++++++++++++++------------------------- lustre/tests/sanityn.sh | 2 +- 2 files changed, 73 insertions(+), 82 deletions(-) diff --git a/lustre/ptlrpc/nrs_tbf.c b/lustre/ptlrpc/nrs_tbf.c index 6ca8588..068da07 100644 --- a/lustre/ptlrpc/nrs_tbf.c +++ b/lustre/ptlrpc/nrs_tbf.c @@ -1819,73 +1819,66 @@ nrs_tbf_generic_cmd_fini(struct nrs_tbf_cmd *cmd) strlen(cmd->u.tc_start.ts_conds_str) + 1); } -#define NRS_TBF_DISJUNCTION_DELIM (',') -#define NRS_TBF_CONJUNCTION_DELIM ('&') -#define NRS_TBF_EXPRESSION_DELIM ('=') - -static inline bool -nrs_tbf_check_field(struct cfs_lstr *field, char *str) -{ - int len = strlen(str); - - return (field->ls_len == len && - strncmp(field->ls_str, str, len) == 0); -} +#define NRS_TBF_DISJUNCTION_DELIM (",") +#define NRS_TBF_CONJUNCTION_DELIM ("&") +#define NRS_TBF_EXPRESSION_DELIM ("=") static int nrs_tbf_opcode_list_parse(char *str, int len, unsigned long **bitmaptr); static int -nrs_tbf_id_list_parse(char *str, int len, struct list_head *id_list, +nrs_tbf_id_list_parse(char *str, struct list_head *id_list, enum nrs_tbf_flag tif); static int -nrs_tbf_expression_parse(struct cfs_lstr *src, struct list_head *cond_list) +nrs_tbf_expression_parse(char *str, struct list_head *cond_list) { struct nrs_tbf_expression *expr; - struct cfs_lstr field; + char *field; int rc = 0; + int len; OBD_ALLOC_PTR(expr); if (expr == NULL) return -ENOMEM; - rc = cfs_gettok(src, NRS_TBF_EXPRESSION_DELIM, &field); - if (rc == 0 || !src->ls_str || src->ls_len <= 2 || - src->ls_str[0] != '{' || src->ls_str[src->ls_len - 1] != '}') + field = strim(strsep(&str, NRS_TBF_EXPRESSION_DELIM)); + if (!*field || !str) + /* No LHS or no '=' sign */ + GOTO(out, rc = -EINVAL); + str = strim(str); + len = strlen(str); + if (len < 2 || str[0] != '{' || str[len-1] != '}') + /* No {} around RHS */ GOTO(out, rc = -EINVAL); /* Skip '{' and '}' */ - src->ls_str++; - src->ls_len -= 2; + str[len-1] = '\0'; + str += 1; + len -= 2; - if (nrs_tbf_check_field(&field, "nid")) { - if (cfs_parse_nidlist(src->ls_str, - src->ls_len, + if (strcmp(field, "nid") == 0) { + if (cfs_parse_nidlist(str, len, &expr->te_cond) <= 0) GOTO(out, rc = -EINVAL); expr->te_field = NRS_TBF_FIELD_NID; - } else if (nrs_tbf_check_field(&field, "jobid")) { - if (nrs_tbf_jobid_list_parse(src->ls_str, - src->ls_len, + } else if (strcmp(field, "jobid") == 0) { + if (nrs_tbf_jobid_list_parse(str, len, &expr->te_cond) < 0) GOTO(out, rc = -EINVAL); expr->te_field = NRS_TBF_FIELD_JOBID; - } else if (nrs_tbf_check_field(&field, "opcode")) { - if (nrs_tbf_opcode_list_parse(src->ls_str, - src->ls_len, + } else if (strcmp(field, "opcode") == 0) { + if (nrs_tbf_opcode_list_parse(str, len, &expr->te_opcodes) < 0) GOTO(out, rc = -EINVAL); expr->te_field = NRS_TBF_FIELD_OPCODE; - } else if (nrs_tbf_check_field(&field, "uid")) { - if (nrs_tbf_id_list_parse(src->ls_str, - src->ls_len, + } else if (strcmp(field, "uid") == 0) { + if (nrs_tbf_id_list_parse(str, &expr->te_cond, NRS_TBF_FLAG_UID) < 0) GOTO(out, rc = -EINVAL); expr->te_field = NRS_TBF_FIELD_UID; - } else if (nrs_tbf_check_field(&field, "gid")) { - if (nrs_tbf_id_list_parse(src->ls_str, - src->ls_len, + } else if (strcmp(field, "gid") == 0) { + if (nrs_tbf_id_list_parse(str, &expr->te_cond, NRS_TBF_FLAG_GID) < 0) GOTO(out, rc = -EINVAL); @@ -1902,10 +1895,9 @@ out: } static int -nrs_tbf_conjunction_parse(struct cfs_lstr *src, struct list_head *cond_list) +nrs_tbf_conjunction_parse(char *str, struct list_head *cond_list) { struct nrs_tbf_conjunction *conjunction; - struct cfs_lstr expr; int rc = 0; OBD_ALLOC_PTR(conjunction); @@ -1915,40 +1907,34 @@ nrs_tbf_conjunction_parse(struct cfs_lstr *src, struct list_head *cond_list) INIT_LIST_HEAD(&conjunction->tc_expressions); list_add_tail(&conjunction->tc_linkage, cond_list); - while (src->ls_str) { - rc = cfs_gettok(src, NRS_TBF_CONJUNCTION_DELIM, &expr); - if (rc == 0) { - rc = -EINVAL; - break; - } - rc = nrs_tbf_expression_parse(&expr, + while (str && !rc) { + char *expr = strsep(&str, NRS_TBF_CONJUNCTION_DELIM); + + rc = nrs_tbf_expression_parse(expr, &conjunction->tc_expressions); - if (rc) - break; } return rc; } static int -nrs_tbf_conds_parse(char *str, int len, struct list_head *cond_list) +nrs_tbf_conds_parse(char *orig, struct list_head *cond_list) { - struct cfs_lstr src; - struct cfs_lstr res; + char *str; int rc = 0; - src.ls_str = str; - src.ls_len = len; + orig = kstrdup(orig, GFP_KERNEL); + if (!orig) + return -ENOMEM; + str = orig; + INIT_LIST_HEAD(cond_list); - while (src.ls_str) { - rc = cfs_gettok(&src, NRS_TBF_DISJUNCTION_DELIM, &res); - if (rc == 0) { - rc = -EINVAL; - break; - } - rc = nrs_tbf_conjunction_parse(&res, cond_list); - if (rc) - break; + while (str && !rc) { + char *term = strsep(&str, NRS_TBF_DISJUNCTION_DELIM); + + rc = nrs_tbf_conjunction_parse(term, cond_list); } + kfree(orig); + return rc; } @@ -1965,7 +1951,6 @@ nrs_tbf_generic_parse(struct nrs_tbf_cmd *cmd, const char *id) /* Parse hybird NID and JOBID conditions */ rc = nrs_tbf_conds_parse(cmd->u.tc_start.ts_conds_str, - strlen(cmd->u.tc_start.ts_conds_str), &cmd->u.tc_start.ts_conds); if (rc) nrs_tbf_generic_cmd_fini(cmd); @@ -2056,7 +2041,6 @@ nrs_tbf_rule_init(struct ptlrpc_nrs_policy *policy, INIT_LIST_HEAD(&rule->tr_conds); if (!list_empty(&start->u.tc_start.ts_conds)) { rc = nrs_tbf_conds_parse(rule->tr_conds_str, - strlen(rule->tr_conds_str), &rule->tr_conds); } if (rc) @@ -2558,37 +2542,42 @@ static void nrs_tbf_id_cmd_fini(struct nrs_tbf_cmd *cmd) } static int -nrs_tbf_id_list_parse(char *str, int len, struct list_head *id_list, +nrs_tbf_id_list_parse(char *orig, struct list_head *id_list, enum nrs_tbf_flag tif) { - struct cfs_lstr src; - struct cfs_lstr res; int rc = 0; + unsigned long val; + char *str; struct tbf_id id = { 0 }; ENTRY; if (tif != NRS_TBF_FLAG_UID && tif != NRS_TBF_FLAG_GID) RETURN(-EINVAL); - src.ls_str = str; - src.ls_len = len; + orig = kstrdup(orig, GFP_KERNEL); + if (!orig) + return -ENOMEM; + INIT_LIST_HEAD(id_list); - while (src.ls_str) { + for (str = orig; str ; ) { struct nrs_tbf_id *nti_id; + char *tok; - if (cfs_gettok(&src, ' ', &res) == 0) - GOTO(out, rc = -EINVAL); + tok = strsep(&str, " "); + if (!*tok) + /* Empty token - leading, trailing, or + * multiple spaces in list + */ + continue; id.ti_type = tif; - if (tif == NRS_TBF_FLAG_UID) { - if (!cfs_str2num_check(res.ls_str, res.ls_len, - &id.ti_uid, 0, (u32)~0U)) - GOTO(out, rc = -EINVAL); - } else { - if (!cfs_str2num_check(res.ls_str, res.ls_len, - &id.ti_gid, 0, (u32)~0U)) - GOTO(out, rc = -EINVAL); - } + rc = kstrtoul(tok, 0, &val); + if (rc < 0) + GOTO(out, rc = -EINVAL); + if (tif == NRS_TBF_FLAG_UID) + id.ti_uid = val; + else + id.ti_gid = val; OBD_ALLOC_PTR(nti_id); if (nti_id == NULL) @@ -2597,7 +2586,11 @@ nrs_tbf_id_list_parse(char *str, int len, struct list_head *id_list, nti_id->nti_id = id; list_add_tail(&nti_id->nti_linkage, id_list); } + if (list_empty(id_list)) + /* Only white space in the list */ + GOTO(out, rc = -EINVAL); out: + kfree(orig); if (rc) nrs_tbf_id_list_free(id_list); RETURN(rc); @@ -2626,7 +2619,6 @@ static int nrs_tbf_ug_id_parse(struct nrs_tbf_cmd *cmd, char *id) strlcpy(cmd->u.tc_start.ts_ids_str, src.ls_str, src.ls_len + 1); rc = nrs_tbf_id_list_parse(cmd->u.tc_start.ts_ids_str, - strlen(cmd->u.tc_start.ts_ids_str), &cmd->u.tc_start.ts_ids, tif); if (rc) nrs_tbf_id_cmd_fini(cmd); @@ -2656,7 +2648,6 @@ nrs_tbf_id_rule_init(struct ptlrpc_nrs_policy *policy, if (!list_empty(&start->u.tc_start.ts_ids)) { rc = nrs_tbf_id_list_parse(rule->tr_ids_str, - strlen(rule->tr_ids_str), &rule->tr_ids, tif); if (rc) CERROR("%ss {%s} illegal\n", diff --git a/lustre/tests/sanityn.sh b/lustre/tests/sanityn.sh index 71c3009..562459d 100755 --- a/lustre/tests/sanityn.sh +++ b/lustre/tests/sanityn.sh @@ -4628,7 +4628,7 @@ test_77q() { for i in {1..50}; do local pid1 pid2 - do_facet mds1 $LCTL set_param mds.MDS.mdt.nrs_tbf_rule="'start rule77q_1 uid={500}&gid={500} rate=100'" & + do_facet mds1 $LCTL set_param mds.MDS.mdt.nrs_tbf_rule="'start rule77q_1 uid={ 500 11 3}&gid={500 10 33 100 } rate=100'" & pid1=$! do_facet mds1 $LCTL set_param mds.MDS.mdt.nrs_tbf_rule="'start rule77q_2 uid={1000}&gid={1000} rate=100'" & pid2=$! -- 1.8.3.1