Hi Babu,
On 8/21/24 1:11 PM, Moger, Babu wrote:
On 8/16/24 17:33, Reinette Chatre wrote:
On 8/6/24 3:00 PM, Babu Moger wrote:
...
+
+static int rdtgroup_str_to_mon_state(char *flag)
+{
+ int i, mon_state = 0;
+
+ for (i = 0; i < strlen(flag); i++) {
+ switch (*(flag + i)) {
+ case 't':
+ mon_state |= ASSIGN_TOTAL;
+ break;
+ case 'l':
+ mon_state |= ASSIGN_LOCAL;
+ break;
+ case '_':
+ mon_state = ASSIGN_NONE;
+ break;
It looks like this supports flags like "_lt", treating it as assigning
both local and total. I expect this should remove all flags instead?
This is a cobination of flags.
"_lt" This will assign both local and total.
"lt_" This with remove both the flags.
It seems alright to me. Do you want me to change the bahaviour here?
This looks like undefined behavior to me. A request to set individual flags
and also clear all flags looks like a contradiction to me.
+ default:
+ break;
+ }
+ }
+
+ return mon_state;
+}
hmmm ... so you removed assigning mon_state to ASSIGN_NONE from default,
but that did not change what this function returns since ASSIGN_NONE is 0
and mon_state is initialized to 0. Unknown flags should cause error so
that it is possible to add flags in the future. Above prevents us from
ever adding new flags.
May be I am missing something here. How about this?
enum {
ASSIGN_NONE = 0,
ASSIGN_TOTAL,
ASSIGN_LOCAL,
ASSIGN_INVALID,
};
static int rdtgroup_str_to_mon_state(char *flag)
{
int i, mon_state = ASSIGN_NONE;
for (i = 0; i < strlen(flag); i++) {
switch (*(flag + i)) {
case 't':
mon_state |= ASSIGN_TOTAL;
break;
case 'l':
mon_state |= ASSIGN_LOCAL;
break;
case '_':
mon_state = ASSIGN_NONE;
break;
default:
mon_state = ASSIGN_INVALID;
goto out_done;
}
}
:out_done:
return mon_state;
}
Then handle the ASSIGN_INVALID from the caller. Is that what you think?
Why not return an error?
+
+static struct rdtgroup *rdtgroup_find_grp(enum rdt_group_type rtype,
char *p_grp, char *c_grp)
rdtgroup_find_grp() -> rdtgroup_find_grp_by_name()?
+{
+ struct rdtgroup *rdtg, *crg;
+
+ if (rtype == RDTCTRL_GROUP && *p_grp == '\0') {
+ return &rdtgroup_default;
+ } else if (rtype == RDTCTRL_GROUP) {
+ list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list)
+ if (!strcmp(p_grp, rdtg->kn->name))
+ return rdtg;
+ } else if (rtype == RDTMON_GROUP) {
+ list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list) {
+ if (!strcmp(p_grp, rdtg->kn->name)) {
+ list_for_each_entry(crg, &rdtg->mon.crdtgrp_list,
+ mon.crdtgrp_list) {
+ if (!strcmp(c_grp, crg->kn->name))
+ return crg;
+ }
+ }
+ }
+ }
+
+ return NULL;
+}
+
+static int rdtgroup_process_flags(struct rdt_resource *r,
+ enum rdt_group_type rtype,
+ char *p_grp, char *c_grp, char *tok)
+{
+ int op, mon_state, assign_state, unassign_state;
+ char *dom_str, *id_str, *op_str;
+ struct rdt_mon_domain *d;
+ struct rdtgroup *rdtgrp;
+ unsigned long dom_id;
+ int ret, found = 0;
+
+ rdtgrp = rdtgroup_find_grp(rtype, p_grp, c_grp);
+
+ if (!rdtgrp) {
+ rdt_last_cmd_puts("Not a valid resctrl group\n");
+ return -EINVAL;
+ }
+
+next:
+ if (!tok || tok[0] == '\0')
+ return 0;
+
+ /* Start processing the strings for each domain */
+ dom_str = strim(strsep(&tok, ";"));
+
+ op_str = strpbrk(dom_str, "=+-");
+
+ if (op_str) {
+ op = *op_str;
+ } else {
+ rdt_last_cmd_puts("Missing operation =, +, -, _ character\n");
+ return -EINVAL;
+ }
+
+ id_str = strsep(&dom_str, "=+-");
+
+ /* Check for domain id '*' which means all domains */
+ if (id_str && *id_str == '*') {
+ d = NULL;
+ goto check_state;
+ } else if (!id_str || kstrtoul(id_str, 10, &dom_id)) {
+ rdt_last_cmd_puts("Missing domain id\n");
+ return -EINVAL;
+ }
+
+ /* Verify if the dom_id is valid */
+ list_for_each_entry(d, &r->mon_domains, hdr.list) {
+ if (d->hdr.id == dom_id) {
+ found = 1;
+ break;
+ }
+ }
+
+ if (!found) {
+ rdt_last_cmd_printf("Invalid domain id %ld\n", dom_id);
+ return -EINVAL;
+ }
+
+check_state:
+ mon_state = rdtgroup_str_to_mon_state(dom_str);
Function should return error and exit here.
No. This is case to skip checking for domain when '*' is passed to apply
assignment to all the domains.
Using "*" for a domain still requires valid flags, no?
Reinette
