[ovs-dev] [PATCH 7/9] flow: Refactor parse_ct_state()

[Greg Rose]

On 08/25/2017 03:51 PM, Yi-Hung Wei wrote:
> Refactor parse_ct_state() to support different delimiters.

Why?  I don't see any use of different delimiters in this patch
and if subsequent patches do use different delimiters then that
should be explained here I think.

> 
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> ---
>   lib/flow.c                   | 6 +++---
>   lib/flow.h                   | 2 +-
>   ofproto/ofproto-dpif-trace.c | 2 +-
>   ovn/utilities/ovn-trace.c    | 2 +-
>   4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index b2b10aa488be..34bc176e8b6e 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -1129,14 +1129,14 @@ ct_state_from_string(const char *s)
>    * returns false, and reports error message in 'ds'. */
>   bool
>   parse_ct_state(const char *state_str, uint32_t default_state,
> -               uint32_t *ct_state, struct ds *ds)
> +               const char *delimiters, uint32_t *ct_state, struct ds *ds)
>   {
>       uint32_t state = default_state;
>       char *state_s = xstrdup(state_str);
>       char *save_ptr = NULL;
>   
> -    for (char *cs = strtok_r(state_s, ", ", &save_ptr); cs;
> -         cs = strtok_r(NULL, ", ", &save_ptr)) {
> +    for (char *cs = strtok_r(state_s, delimiters, &save_ptr); cs;
> +         cs = strtok_r(NULL, delimiters, &save_ptr)) {
>           uint32_t bit = ct_state_from_string(cs);
>           if (!bit) {
>               ds_put_format(ds, "%s: unknown connection tracking state flag",
> diff --git a/lib/flow.h b/lib/flow.h
> index 6ae5a674df46..d1aba31290f7 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -76,7 +76,7 @@ void flow_get_metadata(const struct flow *, struct match *flow_metadata);
>   const char *ct_state_to_string(uint32_t state);
>   uint32_t ct_state_from_string(const char *);
>   bool parse_ct_state(const char *state_str, uint32_t default_state,
> -                    uint32_t *ct_state, struct ds *);
> +                    const char *delimiters, uint32_t *ct_state, struct ds *);
>   bool validate_ct_state(uint32_t state, struct ds *);
>   void flow_clear_conntrack(struct flow *);
>   
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index a86cf211803e..9303ea18c237 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -199,7 +199,7 @@ parse_oftrace_options(int argc, const char *argv[],
>               }
>   
>               uint32_t ct_state;
> -            if (!parse_ct_state(argv[++k], 0, &ct_state, &ds)) {
> +            if (!parse_ct_state(argv[++k], 0, ", ", &ct_state, &ds)) {
>                   return ds_steal_cstr(&ds);
>               }
>               if (!validate_ct_state(ct_state, &ds)) {
> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> index 59083eebe270..9f4cbf58d3e6 100644
> --- a/ovn/utilities/ovn-trace.c
> +++ b/ovn/utilities/ovn-trace.c
> @@ -173,7 +173,7 @@ parse_ct_option(const char *state_s_)
>       uint32_t state;
>       struct ds ds = DS_EMPTY_INITIALIZER;
>   
> -    if (!parse_ct_state(state_s_, CS_TRACKED, &state, &ds)) {
> +    if (!parse_ct_state(state_s_, CS_TRACKED, ", ", &state, &ds)) {
>           ovs_fatal(0, "%s", ds_cstr(&ds));
>       }
>       if (!validate_ct_state(state, &ds)) {
> 

Other than my comment above this patch looks fine.

Reviewed-by: Greg Rose <gvrose8192 at gmail.com>