On 01/03/2023 12:53, Roi Dayan via dev wrote:
>
>
> On 16/01/2023 13:45, Ales Musil wrote:
>> Currently, the CT can be flushed by dpctl only by specifying
>> the whole 5-tuple. This is not very convenient when there are
>> only some fields known to the user of CT flush. Add new struct
>> ofputil_ct_match which represents the generic filtering that can
>> be done for CT flush. The match is done only on fields that are
>> non-zero with exception to the icmp fields.
>>
>> This allows the filtering just within dpctl, however
>> it is a preparation for OpenFlow extension.
>>
>> Reported-at:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2F2120546&data=05%7C01%7Croid%40nvidia.com%7Cbc4c95a2472a4dfbb0a408db1a434035%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638132648384230198%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wBA5yCdijnN%2FkcwxncDe1xxHzsqzGQgCRqNRHIMQUHQ%3D&reserved=0
>> Signed-off-by: Ales Musil <[email protected]>
>> ---
...
>> @@ -1707,37 +1708,55 @@ dpctl_flush_conntrack(int argc, const char *argv[],
>> struct dpctl_params *dpctl_p)
>> {
>> struct dpif *dpif = NULL;
>> - struct ct_dpif_tuple tuple, *ptuple = NULL;
>> + struct ofp_ct_match match = {0};
>> struct ds ds = DS_EMPTY_INITIALIZER;
>> uint16_t zone, *pzone = NULL;
>> int error;
>> int args = argc - 1;
>>
>> - /* Parse ct tuple */
>> - if (args && ct_dpif_parse_tuple(&tuple, argv[args], &ds)) {
>> - ptuple = &tuple;
>> + /* Parse zone. */
>> + if (args && !strncmp(argv[1], "zone=", 5)) {
>> + if (!ovs_scan(argv[1], "zone=%"SCNu16, &zone)) {
>> + ds_put_cstr(&ds, "failed to parse zone");
>> + error = EINVAL;
>> + goto error;
>> + }
>> + pzone = &zone;
>> args--;
>> }
>>
>> - /* Parse zone */
>> - if (args && ovs_scan(argv[args], "zone=%"SCNu16, &zone)) {
>> - pzone = &zone;
>> + /* Parse ct tuples. */
>> + for (int i = 0; i < 2; i++) {
>> + if (!args) {
>> + break;
>> + }
>> +
>> + struct ofp_ct_tuple *tuple =
>> + i ? &match.tuple_reply : &match.tuple_orig;
>> + const char *arg = argv[argc - args];
>> +
>> + if (arg[0] && !ofp_ct_tuple_parse(tuple, arg, &ds, &match.ip_proto,
>> + &match.l3_type)) {
>> + error = EINVAL;
>> + goto error;
>> + }
>> args--;
>> }
>
> Hi,
>
> There is a problem with the change here that you cannot specify now dp.
>
> # ovs-appctl dpctl/flush-conntrack system@ovs-system
> ovs-dpctl: field system@ovs-system missing value (Invalid argument)
>
> As I understand it the old logic was parsing from the end and if we left
> with 1 arg it's considered the dp.
>
> The new logic starts from first argument and fails on first failure
> so we actually never reach the following check if args > 1.
>
> A quick fix I tried is to goto error only if args > 1.
> but this leaves an opening that specifying wrong key fails on the dp arg.
>
> # ovs-dpctl flush-conntrack system@ovs-system key=val
> ovs-dpctl: field system@ovs-system missing value (Invalid argument)
>
> I ended up with parsing backwards and checking if the error is on last
> arg or not.
>
> I'll send a fixup for review.
>
> Thanks,
> Roi
>
Well it worked for manual run I tested but I fail the ct flush testsuite.
53: conntrack - ct flush FAILED
(system-traffic.at:2364)
I'm having some trouble following the log maybe you can help and do the correct
fix.
i'll also try to look but reporting this for now.
The change I tested is the following:
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1737,12 +1737,15 @@ dpctl_flush_conntrack(int argc, const char *argv[],
struct ofp_ct_tuple *tuple =
i ? &match.tuple_reply : &match.tuple_orig;
- const char *arg = argv[argc - args];
+ const char *arg = argv[args];
if (arg[0] && !ofp_ct_tuple_parse(tuple, arg, &ds, &match.ip_proto,
&match.l3_type)) {
- error = EINVAL;
- goto error;
+ if (args > 1) {
+ error = EINVAL;
+ goto error;
+ }
+ break;
}
args--;
}
>>
>> - /* Report error if there are more than one unparsed argument. */
>> + /* Report error if there is more than one unparsed argument. */
>> if (args > 1) {
>> ds_put_cstr(&ds, "invalid arguments");
>> error = EINVAL;
>> goto error;
>> }
>>
>> - error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
>> + error = opt_dpif_open(argc, argv, dpctl_p, 5, &dpif);
>> if (error) {
>> + dpctl_error(dpctl_p, error, "Cannot open dpif");
>> return error;
>> }
>>
...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
