On 10/10/23 16:12, Ales Musil wrote:
> Add optional argument to dpctl ct-del-limits called
> "default", which allows to remove the default limit
> making it effectively system default.
>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> NEWS | 3 +++
> lib/dpctl.c | 20 ++++++++++++++------
> tests/system-traffic.at | 25 +++++++++++++++++++++++++
> 3 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 6b45492f1..df98e75a0 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -6,6 +6,9 @@ Post-v3.2.0
> from older version is supported but it may trigger more leader
> elections
> during the process, and error logs complaining unrecognized fields may
> be observed on old nodes.
> + - ovs-dpctl:
We should use ovs-appctl as a section or both. We should not really
promote use of ovs-dpctl.
> + * Support removal of default CT zone limit via ovs-dpctl, e.g.
> + "ovs-appctl dpctl/ct-del-limits default"
A few issues with this statement:
1. It is a bit inconsistent - talks about ovs-dpctl and then gives
ovs-appctl example.
2. And we're in the dpctl section, there is no need to repeat the
command name two more times.
3. NEWS entry should generally be phrased as an answer to the
'What changed?' question, i.e. 'Added support ...', 'Command <>
now supports ...', etc.
4. Missing period at the end.
>
>
> v3.2.0 - 17 Aug 2023
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index ad104372e..7113c2c12 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -2291,14 +2291,22 @@ dpctl_ct_del_limits(int argc, const char *argv[],
> int i = dp_arg_exists(argc, argv) ? 2 : 1;
> struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits);
>
> - error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
> + error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
> if (error) {
> return error;
> }
>
> - error = parse_ct_limit_zones(argv[i], &zone_limits, &ds);
> - if (error) {
> - goto error;
> + /* Parse default limit */
Period at the end of a comment.
> + if (!strcmp(argv[i], "default")) {
> + ct_dpif_push_zone_limit(&zone_limits, CT_DPIF_DEFAULT_ZONE, 0, 0);
> + i++;
> + }
> +
> + if (argc > i) {
> + error = parse_ct_limit_zones(argv[i], &zone_limits, &ds);
> + if (error) {
> + goto error;
> + }
> }
>
> error = ct_dpif_del_limits(dpif, &zone_limits);
> @@ -3030,8 +3038,8 @@ static const struct dpctl_command all_commands[] = {
> { "ct-get-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_get_tcp_seq_chk, DP_RO },
> { "ct-set-limits", "[dp] [default=L] [zone=N,limit=L]...", 1, INT_MAX,
> dpctl_ct_set_limits, DP_RO },
> - { "ct-del-limits", "[dp] zone=N1[,N2]...", 1, 2, dpctl_ct_del_limits,
> - DP_RO },
> + { "ct-del-limits", "[dp] [default] [zone=N1[,N2]...]", 1, 3,
> + dpctl_ct_del_limits, DP_RO },
Hmm, not a problem of this patch, but both set and del commands
seem to incorrectly report themselves as read-only...
> { "ct-get-limits", "[dp] [zone=N1[,N2]...]", 0, 2, dpctl_ct_get_limits,
> DP_RO },
> { "ct-get-sweep-interval", "[dp]", 0, 1, dpctl_ct_get_sweep, DP_RO },
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 418cd32fe..f35cfaad9 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -5195,6 +5195,31 @@
> udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10.
>
> udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3
> ])
>
> +dnl Test ct-del-limits for default zone.
Maybe add an empty line here, because this comment is not describing
the next immediate block of commands, but applies to all the command
til the end of a test instead.
> +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=4,limit=4])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl
> +default limit=15
> +zone=4,limit=4,count=0
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-del-limits default])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl
> +default limit=0
> +zone=4,limit=4,count=0
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl
> +default limit=15
> +zone=4,limit=4,count=0
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-del-limits default zone=4])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl
> +default limit=0
> +zone=4,limit=0,count=0
> +])
> +
> OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> /could not create datapath/d
> /(Cannot allocate memory) on packet/d"])
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
