[ovs-dev] [RFC 1/2] datapath: Refactor ovs_flow_cmd_fill_info().

[Pravin Shelar]

On Wed, Jul 9, 2014 at 10:29 PM, Joe Stringer <joestringer at nicira.com> wrote:
> Split up ovs_flow_cmd_fill_info() to make it easier to cache parts of a
> dump reply. This will be used to streamline flow_dump in the next patch.
>
Nice refactoring.

> Signed-off-by: Joe Stringer <joestringer at nicira.com>
> ---
>  datapath/datapath.c |   87 ++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 69 insertions(+), 18 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 136a478..bdbcbd5 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -696,26 +696,13 @@ static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts)
>  }
>
>  /* Called with ovs_mutex or RCU read lock. */
> -static int ovs_flow_cmd_fill_info(struct datapath *dp,
> -                                 const struct sw_flow *flow, int dp_ifindex,
> -                                 struct sk_buff *skb, u32 portid,
> -                                 u32 seq, u32 flags, u8 cmd)
> +static int ovs_flow_cmd_fill_match(struct datapath *dp,
> +                                  const struct sw_flow *flow,
> +                                  struct sk_buff *skb)
>  {
> -       const int skb_orig_len = skb->len;
> -       struct nlattr *start;
> -       struct ovs_flow_stats stats;
> -       __be16 tcp_flags;
> -       unsigned long used;
> -       struct ovs_header *ovs_header;
>         struct nlattr *nla;
>         int err;
>
> -       ovs_header = genlmsg_put(skb, portid, seq, &dp_flow_genl_family, flags, cmd);
> -       if (!ovs_header)
> -               return -EMSGSIZE;
> -
> -       ovs_header->dp_ifindex = dp_ifindex;
> -
>         /* Fill flow key. */
>         nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
>         if (!nla)
> @@ -727,6 +714,7 @@ static int ovs_flow_cmd_fill_info(struct datapath *dp,
>                 goto error;
>         nla_nest_end(skb, nla);
>
> +       /* Fill flow mask. */
>         nla = nla_nest_start(skb, OVS_FLOW_ATTR_MASK);
>         if (!nla)
>                 goto nla_put_failure;
> @@ -734,9 +722,25 @@ static int ovs_flow_cmd_fill_info(struct datapath *dp,
>         err = ovs_nla_put_flow(dp, &flow->key, &flow->mask->key, skb);
>         if (err)
>                 goto error;
> -
>         nla_nest_end(skb, nla);
>
> +       return 0;
> +
> +nla_put_failure:
> +       err = -EMSGSIZE;
> +error:
> +       return err;
> +}
> +

The error path are just returning error, so I think we can just return
from error case rather than jumping over goto statement. Similarly we
can avoid goto in newly added following functions.

> +/* Called with ovs_mutex or RCU read lock. */
> +static int ovs_flow_cmd_fill_stats(const struct sw_flow *flow,
> +                                  struct sk_buff *skb)
> +{
> +       struct ovs_flow_stats stats;
> +       __be16 tcp_flags;
> +       unsigned long used;
> +       int err;
> +
>         ovs_flow_stats_get(flow, &stats, &used, &tcp_flags);
>
>         if (used &&
> @@ -751,6 +755,20 @@ static int ovs_flow_cmd_fill_info(struct datapath *dp,
>              nla_put_u8(skb, OVS_FLOW_ATTR_TCP_FLAGS, (u8)ntohs(tcp_flags)))
>                 goto nla_put_failure;
>
> +       return 0;
> +
> +nla_put_failure:
> +       err = -EMSGSIZE;
> +       return err;
> +}
> +
> +/* Called with ovs_mutex or RCU read lock. */
> +static int ovs_flow_cmd_fill_actions(const struct sw_flow *flow,
> +                                    struct sk_buff *skb, int skb_orig_len)
> +{
> +       struct nlattr *start;
> +       int err;
> +
>         /* If OVS_FLOW_ATTR_ACTIONS doesn't fit, skip dumping the actions if
>          * this is the first flow to be dumped into 'skb'.  This is unusual for
>          * Netlink but individual action lists can be longer than
> @@ -780,11 +798,44 @@ static int ovs_flow_cmd_fill_info(struct datapath *dp,
>         } else if (skb_orig_len)
>                 goto nla_put_failure;
>
I know you are not changing this code, but can you add {} to "else if
block" to fix kernel coding standard error.

> -       return genlmsg_end(skb, ovs_header);
> +       return 0;
>
>  nla_put_failure:
>         err = -EMSGSIZE;
>  error:
> +       return err;
> +}
> +
> +/* Called with ovs_mutex or RCU read lock. */
> +static int ovs_flow_cmd_fill_info(struct datapath *dp,
> +                                 const struct sw_flow *flow, int dp_ifindex,
> +                                 struct sk_buff *skb, u32 portid,
> +                                 u32 seq, u32 flags, u8 cmd)
> +{
> +       const int skb_orig_len = skb->len;
> +       struct ovs_header *ovs_header;
> +       int err;
> +
> +       ovs_header = genlmsg_put(skb, portid, seq, &dp_flow_genl_family, flags, cmd);
> +       if (!ovs_header)
> +               return -EMSGSIZE;
> +       ovs_header->dp_ifindex = dp_ifindex;
> +
> +       err = ovs_flow_cmd_fill_match(dp, flow, skb);
> +       if (err)
> +               goto error;
> +
> +       err = ovs_flow_cmd_fill_stats(flow, skb);
> +       if (err)
> +               goto error;
> +
> +       err = ovs_flow_cmd_fill_actions(flow, skb, skb_orig_len);
> +       if (err)
> +               goto error;
> +
> +       return genlmsg_end(skb, ovs_header);
> +
> +error:
>         genlmsg_cancel(skb, ovs_header);
>         return err;
>  }
otherwise looks good.

Acked-by: Pravin B Shelar <pshelar at nicira.com>