Re: [PATCH v5 04/40] x86/resctrl: Use schema type to determine how to parse schema values

[Tony Luck]

On Fri, Oct 04, 2024 at 06:03:11PM +0000, James Morse wrote:
> +static ctrlval_parser_t *get_parser(struct rdt_resource *r)
> +{
> +	switch (r->schema_fmt) {
> +	case RESCTRL_SCHEMA_BITMAP:
> +		return &parse_cbm;
> +	case RESCTRL_SCHEMA_RANGE:
> +		return &parse_bw;
> +	}
> +
> +	return NULL;
> +}

Is it really worth making this a helper function? It's only
used once.

> +
>  /*
>   * For each domain in this resource we expect to find a series of:
>   *	id=mask
> @@ -204,6 +225,7 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>  static int parse_line(char *line, struct resctrl_schema *s,
>  		      struct rdtgroup *rdtgrp)
>  {
> +	ctrlval_parser_t *parse_ctrlval = get_parser(s->res);

No check to see if get_parser() returned NULL.

>  	enum resctrl_conf_type t = s->conf_type;
>  	struct resctrl_staged_config *cfg;
>  	struct rdt_resource *r = s->res;
> @@ -235,7 +257,7 @@ static int parse_line(char *line, struct resctrl_schema *s,
>  		if (d->hdr.id == dom_id) {
>  			data.buf = dom;
>  			data.rdtgrp = rdtgrp;
> -			if (r->parse_ctrlval(&data, s, d))
> +			if (parse_ctrlval(&data, s, d))
>  				return -EINVAL;

Without the helper this could be:

			switch (r->schema_fmt) {
			case RESCTRL_SCHEMA_BITMAP:
				if (parse_cbm(&data, s, d))
					return -EINVAL;
				break;
			case RESCTRL_SCHEMA_RANGE:
				if (parse_bw(&data, s, d))
					return -EINVAL;
				break;
			default:
				WARN_ON_ONCE(1);
				return -EINVAL;
			}

-Tony