On Wed, Jul 9, 2014 at 10:29 PM, Joe Stringer <joestrin...@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 <joestrin...@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 <pshe...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
