Re: [PATCH v2 1/2] net/cpfl: parse flow offloading hint from P4 context file

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

Hi Wenjing,

Please find comments inlined

On 05/01/2024 08:16, wenjing.qiao@intel.com wrote:
> From: Wenjing Qiao <wenjing.qiao@intel.com>
>
> To supporting P4-programmed network controller, reuse devargs
> "flow_parser" to specify the path of a p4 context JSON configure
> file. The cpfl PMD use the JSON configuration file to translate
> rte_flow tokens into low level hardware representation.
>
> Note, the p4 context JSON file is generated by the P4 compiler
> and is intended to work exclusively with a specific P4 pipeline
> configuration, which must be compiled and programmed into the hardware.
>
> Signed-off-by: Wenjing Qiao <wenjing.qiao@intel.com>
> ---
<snip>
> +	if (cpfl_check_is_p4_mode(root)) {
> +		PMD_DRV_LOG(NOTICE, "flow parser mode is p4 mode.");
> +		prog = rte_zmalloc("tdi_parser", sizeof(struct cpfl_tdi_program), 0);
> +		if (prog == NULL) {
> +			PMD_DRV_LOG(ERR, "Failed to create program object.");
> +			return -ENOMEM;
> +		}
> +		ret = cpfl_tdi_program_create(root, prog);
> +		if (ret != 0) {
> +			PMD_INIT_LOG(ERR, "Failed to create tdi program from file %s", filename);
> +			rte_free(prog);

This looks like doublefree to me because cpfl_tdi_program_create() on 
fail calls cpfl_tdi_program_destroy() which in turn does rte_free for 
program.

On a separate note maybe cpfl_tdi_program_create() function should not 
call cpfl_tdi_program_destroy() and maybe it should be responsibility of 
whoever calls _create().


<snip>

> +static int
> +cpfl_tdi_parse_match_key_field_obj(json_t *root, struct cpfl_tdi_match_key_field *mkf)
> +{
> +	int ret, val = 0;
> +	char last3char[4];
> +
> +	ret = cpfl_tdi_get_string_obj(root, "name", mkf->name);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = cpfl_tdi_get_string_obj(root, "instance_name", mkf->instance_name);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = cpfl_tdi_get_string_obj(root, "field_name", mkf->field_name);
> +	if (ret != 0)
> +		return ret;
> +	strncpy(last3char, mkf->field_name + strlen(mkf->field_name) - 3, 3);
> +	last3char[3] = '\0';
> +	if (!strcmp(last3char, "VSI") || !strcmp(last3char, "vsi"))

If this is now case sensitive then I'd suggest to make contents of the 
last3char lower case to handle all variations of "VSi".

> +		mkf->is_vsi = true;
> +	else
> +		mkf->is_vsi = false;
> +
> +	ret = cpfl_tdi_parse_match_type(root, mkf);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = cpfl_tdi_get_integer_obj(root, "bit_width", &val);
> +	if (ret != 0)
> +		return ret;
> +
> +	mkf->bit_width = (uint16_t)val;

Here and several places below, does it need to be range checked?

> +
> +	ret = cpfl_tdi_get_integer_obj(root, "index", &val);
> +	if (ret != 0)
> +		return ret;
> +
> +	mkf->index = (uint32_t)val;
> +
> +	ret = cpfl_tdi_get_integer_obj(root, "position", &val);
> +	if (ret != 0)
> +		return ret;
> +
> +	mkf->position = (uint32_t)val;
> +
> +	return 0;
> +}
> +
> +static int
> +cpfl_tdi_parse_match_key_fields(json_t *root, struct cpfl_tdi_table *table)
> +{
> +	int ret;
> +	int array_len = json_array_size(root);
> +
> +	if (array_len == 0)
> +		return 0;
> +
> +	table->match_key_field_num = (uint16_t)array_len;
> +	table->match_key_fields =
> +	    rte_zmalloc(NULL, sizeof(struct cpfl_tdi_match_key_field) * array_len, 0);
> +	if (table->match_key_fields == NULL) {
> +		PMD_DRV_LOG(ERR, "Failed to create match key field array.");
> +		return -ENOMEM;
> +	}
> +
> +	for (int i = 0; i < array_len; i++) {
> +		json_t *mkf_object = json_array_get(root, i);
> +
> +		ret = cpfl_tdi_parse_match_key_field_obj(mkf_object, &table->match_key_fields[i]);
> +		if (ret != 0)
Possible memory leak due to earlier rte_zmalloc(). There are several 
places below with the same problem.
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +cpfl_tdi_parse_byte_order(json_t *root, struct cpfl_tdi_match_key_format *mkf)
> +{
> +	char bo[CPFL_TDI_JSON_STR_SIZE_MAX];
> +	int ret;
> +
> +	ret = cpfl_tdi_get_string_obj(root, "byte_order", bo);
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	if (!strcmp(bo, "HOST")) {
Is it expected to be upper case always?
> +		mkf->byte_order = CPFL_TDI_BYTE_ORDER_HOST;
> +	} else if (!strcmp(bo, "NETWORK")) {
> +		mkf->byte_order = CPFL_TDI_BYTE_ORDER_NETWORK;
> +	} else {
> +		PMD_DRV_LOG(ERR, "Unknown byte order type %s", bo);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
<snip>
> +
> +static int
> +cpfl_tdi_pparse_action_code(json_t *root, struct cpfl_tdi_hw_action *ha)
double "p"
<snip>
> +static int
> +cpfl_tdi_parse_table_obj(json_t *root, struct cpfl_tdi_table *table)
> +{
> +	int ret, val = 0;
> +	struct json_t *jobj = NULL;
> +
> +	ret = cpfl_tdi_parse_table_type(root, table);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = cpfl_tdi_get_integer_obj(root, "handle", &val);
> +	if (ret != 0)
> +		return ret;
> +	table->handle = (uint32_t)val;
> +
> +	ret = cpfl_tdi_get_string_obj(root, "name", table->name);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (table->table_type == CPFL_TDI_TABLE_TYPE_POLICER_METER) {
> +		/* TODO */
It looks like not implemented yet, should this return 0? Does it need to 
have some kind of logging?
> +		return 0;
> +	}
<snip>
> +static int
> +cpfl_tdi_parse_global_configs(json_t *root, struct cpfl_tdi_global_configs *gc)
> +{
> +	json_t *jobj = NULL;
> +	int ret;
> +
> +	ret = cpfl_tdi_get_array_obj(root, "hardware_blocks", &jobj);
> +	if (ret != 0)
> +		return ret;
> +
> +	return cpfl_tdi_parse_gc_hardware_blocks(jobj, gc);
> +}
> +
> +int
> +cpfl_tdi_program_create(json_t *root, struct cpfl_tdi_program *prog)
Should input parameters be checked against NULL?
> +{
> +	json_t *jobj = NULL;
> +	int ret;
> +
> +	ret = cpfl_tdi_get_string_obj(root, "program_name", prog->program_name);
> +	if (ret != 0)
> +		goto err;
> +
> +	ret = cpfl_tdi_get_string_obj(root, "build_date", prog->build_date);
> +	if (ret != 0)
> +		goto err;
> +
> +	ret = cpfl_tdi_get_string_obj(root, "compile_command", prog->compile_command);
> +	if (ret != 0)
> +		goto err;
> +
> +	ret = cpfl_tdi_get_string_obj(root, "compiler_version", prog->compiler_version);
> +	if (ret != 0)
> +		goto err;
> +
> +	ret = cpfl_tdi_get_string_obj(root, "schema_version", prog->schema_version);
> +	if (ret != 0)
> +		goto err;
> +
> +	ret = cpfl_tdi_get_string_obj(root, "target", prog->target);
> +	if (ret != 0)
> +		goto err;
> +
> +	ret = cpfl_tdi_get_object_obj(root, "global_configs", &jobj);
> +	if (ret != 0)
> +		goto err;
> +
> +	ret = cpfl_tdi_parse_global_configs(jobj, &prog->global_configs);
> +	if (ret != 0)
> +		goto err;
> +
> +	ret = cpfl_tdi_get_array_obj(root, "tables", &jobj);
> +	if (ret != 0)
> +		goto err;
> +
> +	ret = cpfl_tdi_parse_tables(jobj, prog);
> +	if (ret != 0)
> +		goto err;
> +
> +	json_decref(root);
is this json_decref() needed? This function is called from 
cpfl_parser_create() which already does json_decref().
> +
> +	return 0;
> +
> +err:
> +	cpfl_tdi_program_destroy(prog);
> +	return ret;
> +}
> +
<snip>

-- 
Regards,
Vladimir