Hi Wenjing,
Please find comments inlined
On 05/01/2024 08:16, wenjing.q...@intel.com wrote:
From: Wenjing Qiao <wenjing.q...@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.q...@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
