Thanks for the patch Yi-hung


On Wed, Aug 1, 2018 at 3:46 PM, Yi-Hung Wei <[email protected]> wrote:

> This patch implments the following three commands on dpctl so that
> users can use ovs-dpctl or ovs-appctl to set, delete, and get the
> per zone limit.
>
> For example,
>


Do these commands need a ‘zone’ keyword
eg) ct-set-zone-limits



>
> $ ovs-appctl dpctl/ct-set-limits default=10 zone=0,limit=5 zone=1,limit=3
>

I wonder if it makes sense to write

‘zone=default,limit=10’

so that ‘default’ is treated like any zone?



> $ ovs-appctl dpct/ct-del-limits zone=0
>


I wonder if we set zone limit to zero (unlimited), it could be like
deleting a zone limit.

$ ovs-appctl dpctl/ct-set-limits zone=1,limit=0

in which case, we don’t need a special command for delete, such as:

$ ovs-appctl dpct/ct-del-limits zone=1



So, we would have just ‘set’ and ‘get’ commands remaining.




> $ ovs-appctl dpct/ct-get-limits zone=1,2,3
>
> Signed-off-by: Yi-Hung Wei <[email protected]>
> ---
>  NEWS          |   2 +
>  lib/ct-dpif.c |  67 +++++++++++++++++++++++
>  lib/ct-dpif.h |   4 ++
>  lib/dpctl.c   | 169 ++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++-
>  lib/dpctl.man |  18 +++++++
>  5 files changed, 259 insertions(+), 1 deletion(-)
>
> diff --git a/NEWS b/NEWS
> index 8270ef46ea34..31c5a1e400dc 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -19,6 +19,8 @@ v2.10.0 - xx xxx xxxx
>         default it always accepts names and in interactive use it displays
> them;
>         use --names or --no-names to override.  See ovs-ofctl(8) for
> details.
>     - ovs-vsctl: New commands "add-bond-iface" and "del-bond-iface".
> +   - ovs-dpctl:
> +     * New commands "ct-set-limits", "ct-del-limits", and "ct-get-limits".
>     - OpenFlow:
>       * OFPT_ROLE_STATUS is now available in OpenFlow 1.3.
>       * OpenFlow 1.5 extensible statistics (OXS) now implemented.
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index a772799fe347..bb809d9920b5 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -629,3 +629,70 @@ ct_dpif_free_zone_limits(struct ovs_list
> *zone_limits)
>          free(p);
>      }
>  }
> +
> +/* Parses a specification of a conntrack zone limit from 's' into '*pzone'
> + * and '*plimit'.  Returns true on success.  Otherwise, returns false and
> + * and puts the error message in 'ds'. */
> +bool
> +ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone,
> +                               uint32_t *plimit, struct ds *ds)
> +{
> +    char *pos, *key, *value, *copy, *err;
> +    bool parsed_limit = false, parsed_zone = false;
> +
> +    pos = copy = xstrdup(s);
> +    while (ofputil_parse_key_value(&pos, &key, &value)) {
> +        if (!*value) {
> +            ds_put_format(ds, "field %s missing value", key);
> +            goto error;
> +        }
> +
> +        if (!strcmp(key, "zone")) {
> +            err = str_to_u16(value, key, pzone);
> +            if (err) {
> +                free(err);
> +                goto error_with_msg;
> +            }
> +            parsed_zone = true;
> +        }  else if (!strcmp(key, "limit")) {
> +            err = str_to_u32(value, plimit);
> +            if (err) {
> +                free(err);
> +                goto error_with_msg;
> +            }
> +            parsed_limit = true;
> +        } else {
> +            ds_put_format(ds, "invalid zone limit field: %s", key);
> +            goto error;
> +        }
> +    }
> +
> +    if (parsed_zone == false || parsed_limit == false) {
> +        ds_put_format(ds, "failed to parse zone limit");
> +        goto error;
> +    }
> +
> +    free(copy);
> +    return true;
> +
> +error_with_msg:
> +    ds_put_format(ds, "failed to parse field %s", key);
> +error:
> +    free(copy);
> +    return false;
> +}
> +
> +void
> +ct_dpif_format_zone_limits(uint32_t default_limit,
> +                           const struct ovs_list *zone_limits, struct ds
> *ds)
> +{
> +    struct ct_dpif_zone_limit *zone_limit;
> +
> +    ds_put_format(ds, "default_limit=%"PRIu32, default_limit);
> +
> +    LIST_FOR_EACH (zone_limit, node, zone_limits) {
> +        ds_put_format(ds, " zone=%"PRIu16, zone_limit->zone);
> +        ds_put_format(ds, ",limit=%"PRIu32, zone_limit->limit);
> +        ds_put_format(ds, ",count=%"PRIu32, zone_limit->count);
> +    }
> +}
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index c80e18b72b56..c9cfb258b133 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -223,5 +223,9 @@ void ct_dpif_push_zone_limit(struct ovs_list *,
> uint16_t zone, uint32_t limit,
>                               uint32_t count);
>  struct ct_dpif_zone_limit * ct_dpif_pop_zone_limit(struct ovs_list *);
>  void ct_dpif_free_zone_limits(struct ovs_list *);
> +bool ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone,
> +                                    uint32_t *plimit, struct ds *);
> +void ct_dpif_format_zone_limits(uint32_t default_limit,
> +                                const struct ovs_list *, struct ds *);
>
>  #endif /* CT_DPIF_H */
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 35733774b331..560f713cfd51 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -199,7 +199,7 @@ parsed_dpif_open(const char *arg_, bool create, struct
> dpif **dpifp)
>   * to be parsed in '*indexp'.  */
>  static int
>  opt_dpif_open(int argc, const char *argv[], struct dpctl_params *dpctl_p,
> -              uint8_t max_args, struct dpif **dpifp, bool multi_opt,
> +              int max_args, struct dpif **dpifp, bool multi_opt,
>                int *indexp)
>  {
>      char *dpname;
> @@ -1683,6 +1683,167 @@ dpctl_ct_get_nconns(int argc, const char *argv[],
>      return error;
>  }
>
> +static int
> +dpctl_ct_set_limits(int argc, const char *argv[],
> +                    struct dpctl_params *dpctl_p)
> +{
> +    struct dpif *dpif;
> +    struct ds ds = DS_EMPTY_INITIALIZER;
> +    int error, i = 1;
> +    uint32_t default_limit, *p_default_limit = NULL;
> +    struct ovs_list list = OVS_LIST_INITIALIZER(&list);
> +
> +    error = opt_dpif_open(argc, argv, dpctl_p, INT_MAX, &dpif, true, &i);
> +    if (error) {
> +        return error;
> +    }
> +
> +    /* Parse default limit */
> +    if (!strncmp(argv[i], "default=", 8)) {
> +        if (ovs_scan(argv[i], "default=%"SCNu32, &default_limit)) {
> +            p_default_limit = &default_limit;
> +            i++;
> +        } else {
> +            ds_put_cstr(&ds, "invalid default limit");
> +            error = EINVAL;
> +            goto error;
> +        }
> +    }
> +
> +    /* Parse ct zone limit tuples */
> +    while (i < argc) {
> +        uint16_t zone;
> +        uint32_t limit;
> +        if (!ct_dpif_parse_zone_limit_tuple(argv[i++], &zone, &limit,
> &ds)) {
> +            error = EINVAL;
> +            goto error;
> +        }
> +        ct_dpif_push_zone_limit(&list, zone, limit, 0);
> +    }
> +
> +    error = ct_dpif_set_limits(dpif, p_default_limit, &list);
> +    if (!error) {
> +        ct_dpif_free_zone_limits(&list);
> +        dpif_close(dpif);
> +        return 0;
> +    } else {
> +        ds_put_cstr(&ds, "failed to set conntrack limit");
> +    }
> +
> +error:
> +    dpctl_error(dpctl_p, error, "%s", ds_cstr(&ds));
> +    ds_destroy(&ds);
> +    ct_dpif_free_zone_limits(&list);
> +    dpif_close(dpif);
> +    return error;
> +}
> +
> +static int
> +parse_ct_limit_zones(const char *argv, struct ovs_list *list, struct ds
> *ds)
> +{
> +    char *save_ptr = NULL, *argcopy, *next_zone;
> +    uint16_t zone;
> +
> +    if (strncmp(argv, "zone=", 5)) {
> +        ds_put_format(ds, "invalid argument %s", argv);
> +        return EINVAL;
> +    }
> +
> +    argcopy = xstrdup(argv + 5);
> +    next_zone = strtok_r(argcopy, ",", &save_ptr);
> +
> +    do {
> +        if (ovs_scan(next_zone, "%"SCNu16, &zone)) {
> +            ct_dpif_push_zone_limit(list, zone, 0, 0);
> +        } else {
> +            ds_put_cstr(ds, "invalid zone");
> +            return EINVAL;
> +        }
> +    } while ((next_zone = strtok_r(NULL, ",", &save_ptr)) != NULL);
> +
> +    free(argcopy);
> +    return 0;
> +}
> +
> +static int
> +dpctl_ct_del_limits(int argc, const char *argv[],
> +                    struct dpctl_params *dpctl_p)
> +{
> +    struct dpif *dpif;
> +    struct ds ds = DS_EMPTY_INITIALIZER;
> +    int error, i = 1;
> +    struct ovs_list list = OVS_LIST_INITIALIZER(&list);
> +
> +    error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif, true, &i);
> +    if (error) {
> +        return error;
> +    }
> +
> +    error = parse_ct_limit_zones(argv[i], &list, &ds);
> +    if (error) {
> +        goto error;
> +    }
> +
> +    error = ct_dpif_del_limits(dpif, &list);
> +    if (!error) {
> +        goto out;
> +    } else {
> +        ds_put_cstr(&ds, "failed to delete conntrack limit");
> +    }
> +
> +error:
> +    dpctl_error(dpctl_p, error, "%s", ds_cstr(&ds));
> +    ds_destroy(&ds);
> +out:
> +    ct_dpif_free_zone_limits(&list);
> +    dpif_close(dpif);
> +    return error;
> +}
> +
> +static int
> +dpctl_ct_get_limits(int argc, const char *argv[],
> +                    struct dpctl_params *dpctl_p)
> +{
> +    struct dpif *dpif;
> +    struct ds ds = DS_EMPTY_INITIALIZER;
> +    uint32_t default_limit;
> +    int error, i = 1;
> +    struct ovs_list list_query = OVS_LIST_INITIALIZER(&list_query);
> +    struct ovs_list list_reply = OVS_LIST_INITIALIZER(&list_reply);
> +
> +    error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif, true, &i);
> +    if (error) {
> +        return error;
> +    }
> +
> +    if (argc > i) {
> +        error = parse_ct_limit_zones(argv[i], &list_query, &ds);
> +        if (error) {
> +            goto error;
> +        }
> +    }
> +
> +    error = ct_dpif_get_limits(dpif, &default_limit, &list_query,
> +                               &list_reply);
> +    if (!error) {
> +        ct_dpif_format_zone_limits(default_limit, &list_reply, &ds);
> +        dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds));
> +        goto out;
> +    } else {
> +        ds_put_format(&ds, "failed to get conntrack limit %s",
> +                      ovs_strerror(error));
> +    }
> +
> +error:
> +    dpctl_error(dpctl_p, error, "%s", ds_cstr(&ds));
> +out:
> +    ds_destroy(&ds);
> +    ct_dpif_free_zone_limits(&list_query);
> +    ct_dpif_free_zone_limits(&list_reply);
> +    dpif_close(dpif);
> +    return error;
> +}
> +
>  /* Undocumented commands for unit testing. */
>
>  static int
> @@ -1982,6 +2143,12 @@ static const struct dpctl_command all_commands[] = {
>      { "ct-set-maxconns", "[dp] maxconns", 1, 2, dpctl_ct_set_maxconns,
> DP_RW },
>      { "ct-get-maxconns", "[dp]", 0, 1, dpctl_ct_get_maxconns, DP_RO },
>      { "ct-get-nconns", "[dp]", 0, 1, dpctl_ct_get_nconns, 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-get-limits", "[dp] [zone=N1[,N2]...]", 0, 2,
> dpctl_ct_get_limits,
> +        DP_RO },
>      { "help", "", 0, INT_MAX, dpctl_help, DP_RO },
>      { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO },
>
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 5d987e62daaa..deb1bd32a34b 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -272,3 +272,21 @@ Only supported for userspace datapath.
>  \*(DX\fBct\-get\-nconns\fR [\fIdp\fR]
>  Prints the current number of connection tracker entries on \fIdp\fR.
>  Only supported for userspace datapath.
> +.
> +.TP
> +\*(DX\fBct\-set\-limits\fR [\fIdp\fR] [\fBdefault=\fIdefault_limit\fR]
> [\fBzone=\fIzone\fR,\fBlimit=\fIlimit\fR]...
> +Sets the maximum allowed number of connections in connection tracking
> zones.
> +If a per zone limit for a particular zone is not available in the
> datapath,
> +it defaults to the default per zone limit.  Initially, the default per
> zone
> +limit is unlimited(0).
> +.
> +.TP
> +\*(DX\fBct\-del\-limits\fR [\fIdp\fR] \fBzone=\fIzone[,zone]\fR...
> +Deletes the per zone limit on particular zones.  The \fIzone\fR must be a
> +comma-separated list.
> +.
> +.TP
> +\*(DX\fBct\-get\-limits\fR [\fIdp\fR] [\fBzone=\fIzone[,zone]\fR...]
> +Retrieves the maximum allowed number of connections.  The \fIzone\fR must
> +be a comma-separated list.  Prints all zone limits if no \fIzone\fR is
> +provided.
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to