>> ---
>> v2 Changes:
>> - Fix minor formatting error.
>> - Change uint8_t to bool.
>[...]
>> @@ -99,6 +103,69 @@ l2fwd_event_parse_eventq_sched(const char
>*optarg,
>> rsrc->sched_type = RTE_SCHED_TYPE_PARALLEL;
>> }
>>
>> +static int
>> +l2fwd_parse_port_pair_config(const char *q_arg, struct
>l2fwd_resources *rsrc)
>> +{
>> + enum fieldnames {
>> + FLD_PORT1 = 0,
>> + FLD_PORT2,
>> + _NUM_FLD
>> + };
>> + const char *p, *p0 = q_arg;
>> + uint16_t int_fld[_NUM_FLD];
>> + char *str_fld[_NUM_FLD];
>> + uint16_t port_pair = 0;
>> + unsigned int size;
>> + char s[256];
>> + char *end;
>> + int i;
>> +
>> + while ((p = strchr(p0, '(')) != NULL) {
>> + ++p;
>> + p0 = strchr(p, ')');
>> + if (p0 == NULL)
>> + return -1;
>> +
>> + size = p0 - p;
>> + if (size >= sizeof(s))
>> + return -1;
>> +
>> + snprintf(s, sizeof(s), "%.*s", size, p);
>
>This is a bit peculiar form of memcpy - you want no more than sizeof(s)
>copied but that you checked above so here simple memcpy should be
>enough.
This is a remnant of l3fwd --config options parsing, I will change it to memcpy
in
next version.
>
>> + if (rte_strsplit(s, sizeof(s), str_fld,
>> + _NUM_FLD, ',') != _NUM_FLD)
>> + return -1;
>> +
>> + for (i = 0; i < _NUM_FLD; i++) {
>> + errno = 0;
>> + int_fld[i] = strtoul(str_fld[i], &end, 0);
>> + if (errno != 0 || end == str_fld[i] || int_fld[i] >
>255)
>
>Replace 255 with check on ">= RTE_MAX_ETHPORTS".
Will fix in next version.
>
>> + return -1;
>> + }
>> +
>> + if (port_pair >= RTE_MAX_ETHPORTS / 2) {
>> + printf("exceeded max number of port pair
>params: Current %d Max = %d\n",
>> + port_pair, RTE_MAX_ETHPORTS / 2);
>> + return -1;
>> + }
>> +
>> + if ((rsrc->dst_ports[int_fld[FLD_PORT1]] !=
>UINT32_MAX) ||
>> + (rsrc->dst_ports[int_fld[FLD_PORT2]] !=
>UINT32_MAX)) {
>> + printf("Duplicate port pair (%d,%d) config\n",
>> + int_fld[FLD_PORT1],
>int_fld[FLD_PORT2]);
>> + return -1;
>> + }
>> +
>> + rsrc->dst_ports[int_fld[FLD_PORT1]] =
>int_fld[FLD_PORT2];
>> + rsrc->dst_ports[int_fld[FLD_PORT2]] =
>int_fld[FLD_PORT1];
>> +
>> + port_pair++;
>> + }
>> +
>> + rsrc->port_pairs = true;
>> +
>> + return 0;
>> +}
>> +
>[...]
>> @@ -209,6 +293,51 @@ l2fwd_event_parse_args(int argc, char
>**argv,
>> return ret;
>> }
>>
>> +/*
>> + * Check port pair config with enabled port mask,
>> + * and for valid port pair combinations.
>> + */
>> +static int
>> +check_port_pair_config(struct l2fwd_resources *rsrc)
>> +{
>> + uint32_t port_pair_mask = 0;
>> + uint32_t portid;
>> + uint16_t index;
>> +
>> + for (index = 0; index < rte_eth_dev_count_avail(); index++) {
>> + if ((rsrc->enabled_port_mask & (1 << index)) == 0)
>> + continue;
>> +
>> + portid = rsrc->dst_ports[index];
>> + if (portid == UINT32_MAX) {
>> + printf("port %u is enabled in but no valid port
>pair\n",
>> + index);
>> + return -1;
>> + }
>> +
>> + if (!rte_eth_dev_is_valid_port(index)) {
>> + printf("port %u is not valid\n", index);
>> + return -1;
>> + }
>> +
>> + if (!rte_eth_dev_is_valid_port(portid)) {
>> + printf("port %u is not valid\n", portid);
>> + return -1;
>> + }
>> +
>> + if (port_pair_mask & (1 << portid) &&
>> + rsrc->dst_ports[portid] != index) {
>> + printf("port %u is used in other port pairs\n",
>portid);
>> + return -1;
>> + }
>> +
>> + port_pair_mask |= (1 << portid);
>> + port_pair_mask |= (1 << index);
>> + }
>
>In the above loop you are doing checks twice. Suppose you have pair
>(2,3) and you go by index from 0 (like you do) and reach point i=2.
>Then you check i=2 and p=3, then on next iteration you do the same
>checks (this time i=3,p=2). I guess simple fix would be to skip loop
>iteration both on not enabled (like you do) and on check if the port was
>already checked (test bit in port_pair_mask).
Ack, will fix in v3.
>
>With regards
>Andrzej Ostruszka
Thanks,
Pavan.
