On Wed, Oct 4, 2023 at 8:45 AM Ales Musil <amu...@redhat.com> wrote:
> In order to make the command extensible unify the arguments
> parsing into single function. This will be later on used
> for the mark and labels arguments.
>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> include/openvswitch/ofp-ct.h | 5 ++--
> lib/dpctl.c | 41 ++++---------------------------
> lib/ofp-ct.c | 47 +++++++++++++++++++++++++++++++++++-
> utilities/ovs-ofctl.c | 37 ++++++----------------------
> 4 files changed, 61 insertions(+), 69 deletions(-)
>
> diff --git a/include/openvswitch/ofp-ct.h b/include/openvswitch/ofp-ct.h
> index c8023c309..cd6192e6f 100644
> --- a/include/openvswitch/ofp-ct.h
> +++ b/include/openvswitch/ofp-ct.h
> @@ -58,8 +58,9 @@ bool ofp_ct_tuple_is_zero(const struct ofp_ct_tuple *,
> uint8_t ip_proto);
> bool ofp_ct_tuple_is_five_tuple(const struct ofp_ct_tuple *, uint8_t
> ip_proto);
>
> void ofp_ct_match_format(struct ds *, const struct ofp_ct_match *);
> -bool ofp_ct_tuple_parse(struct ofp_ct_tuple *, const char *,
> - struct ds *, uint8_t *ip_proto, uint16_t
> *l3_type);
> +bool ofp_ct_match_parse(const char **, int argc, struct ds *,
> + struct ofp_ct_match *, bool *with_zone,
> + uint16_t *zone_id);
>
> enum ofperr ofp_ct_match_decode(struct ofp_ct_match *, bool *with_zone,
> uint16_t *zone_id, const struct
> ofp_header *);
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index cd12625a1..bbab5881e 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1773,48 +1773,17 @@ dpctl_flush_conntrack(int argc, const char *argv[],
> struct dpif *dpif = NULL;
> struct ofp_ct_match match = {0};
> struct ds ds = DS_EMPTY_INITIALIZER;
> - uint16_t zone, *pzone = NULL;
> + uint16_t zone;
> int error;
> int args = argc - 1;
> - int zone_pos = 1;
> + bool with_zone = false;
>
> if (dp_arg_exists(argc, argv)) {
> args--;
> - zone_pos = 2;
> - }
> -
> - /* Parse zone. */
> - if (args && !strncmp(argv[zone_pos], "zone=", 5)) {
> - if (!ovs_scan(argv[zone_pos], "zone=%"SCNu16, &zone)) {
> - ds_put_cstr(&ds, "failed to parse zone");
> - error = EINVAL;
> - goto error;
> - }
> - pzone = &zone;
> - args--;
> - }
> -
> - /* 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--;
> }
>
> - /* Report error if there is more than one unparsed argument. */
> - if (args > 0) {
> - ds_put_cstr(&ds, "invalid arguments");
> + if (args && !ofp_ct_match_parse(&argv[argc - args], args, &ds, &match,
> + &with_zone, &zone)) {
> error = EINVAL;
> goto error;
> }
> @@ -1825,7 +1794,7 @@ dpctl_flush_conntrack(int argc, const char *argv[],
> return error;
> }
>
> - error = ct_dpif_flush(dpif, pzone, &match);
> + error = ct_dpif_flush(dpif, with_zone ? &zone : NULL, &match);
> if (!error) {
> dpif_close(dpif);
> return 0;
> diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c
> index 85a9d8bec..32aeb5455 100644
> --- a/lib/ofp-ct.c
> +++ b/lib/ofp-ct.c
> @@ -98,7 +98,7 @@ ofp_ct_match_format(struct ds *ds, const struct
> ofp_ct_match *match)
> /* Parses a specification of a conntrack 5-tuple from 's' into 'tuple'.
> * Returns true on success. Otherwise, returns false and puts the error
> * message in 'ds'. */
> -bool
> +static bool
> ofp_ct_tuple_parse(struct ofp_ct_tuple *tuple, const char *s,
> struct ds *ds, uint8_t *ip_proto, uint16_t *l3_type)
> {
> @@ -216,6 +216,51 @@ error:
> return false;
> }
>
> +/* Parses a specification of a conntrack match from 'argv' into 'match'.
> + * Returns true on success. Otherwise, returns false and puts the error
> + * message in 'ds'. */
> +bool
> +ofp_ct_match_parse(const char **argv, int argc, struct ds *ds,
> + struct ofp_ct_match *match, bool *with_zone,
> + uint16_t *zone_id)
> +{
> + int args = argc;
> +
> + /* Parse zone. */
> + if (args && !strncmp(argv[argc - args], "zone=", 5)) {
> + if (!ovs_scan(argv[argc - args], "zone=%"SCNu16, zone_id)) {
> + ds_put_cstr(ds, "failed to parse zone");
> + return false;
> + }
> + *with_zone = true;
> + args--;
> + }
> +
> + /* 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)) {
> + return false;
> + }
> + args--;
> + }
> +
> + if (args > 0) {
> + ds_put_cstr(ds, "invalid arguments");
> + return false;
> + }
> +
> + return true;
> +}
> +
> static enum ofperr
> ofpprop_pull_ipv6(struct ofpbuf *property, struct in6_addr *addr,
> uint16_t *l3_type)
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 24d0941cf..79d42dd0b 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -3060,42 +3060,19 @@ ofctl_ct_flush(struct ovs_cmdl_context *ctx)
> struct vconn *vconn;
> struct ofp_ct_match match = {0};
> struct ds ds = DS_EMPTY_INITIALIZER;
> - uint16_t zone, *pzone = NULL;
> + uint16_t zone;
> int args = ctx->argc - 2;
> + bool with_zone = false;
>
> - /* Parse zone. */
> - if (args && !strncmp(ctx->argv[2], "zone=", 5)) {
> - if (!ovs_scan(ctx->argv[2], "zone=%"SCNu16, &zone)) {
> - ovs_fatal(0, "Failed to parse zone");
> - }
> - pzone = &zone;
> - args--;
> - }
> -
> - /* 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 = ctx->argv[ctx->argc - args];
> -
> - if (arg[0] && !ofp_ct_tuple_parse(tuple, arg, &ds,
> &match.ip_proto,
> - &match.l3_type)) {
> - ovs_fatal(0, "Failed to parse ct-tuple: %s", ds_cstr(&ds));
> - }
> - args--;
> - }
> -
> - if (args > 0) {
> - ovs_fatal(0, "Invalid arguments");
> + if (args && !ofp_ct_match_parse((const char **) &ctx->argv[2],
> + args, &ds, &match, &with_zone,
> &zone)) {
> + ovs_fatal(0, "Failed to parse CT match: %s", ds_cstr(&ds));
> }
>
> open_vconn(ctx->argv[1], &vconn);
> enum ofp_version version = vconn_get_version(vconn);
> - struct ofpbuf *msg = ofp_ct_match_encode(&match, pzone, version);
> + struct ofpbuf *msg =
> + ofp_ct_match_encode(&match, with_zone ? &zone : NULL,
> version);
>
> ds_destroy(&ds);
> transact_noreply(vconn, msg);
> --
> 2.41.0
>
>
I have missed one small detail, it will be in v3. I'll wait for reviews
before posting v3 though.
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 418cd32fe..ec65ca5de 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2737,7 +2737,7 @@ AT_CHECK([ovs-ofctl ct-flush br0 zone=1
'ct_nw_src=10.1.1.1' 'ct_nw_dst=10.1.1.1
AT_CHECK([grep -q "command takes at most 4 arguments" stderr])
AT_CHECK([ovs-ofctl ct-flush br0 'ct_nw_src=10.1.1.1' 'ct_nw_dst=10.1.1.1'
invalid], [1], [ignore], [stderr])
-AT_CHECK([grep -q "Invalid arguments" stderr])
+AT_CHECK([grep -q "invalid arguments" stderr])
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
