It looks good overall. Thanks for working on this.
Signed-off-by: Andy Zhou <az...@nicira.com>
I have a few minor comments:
In datapatch.c, should we extend flow_policy[] for the new flow
attributes? It seems OVS_fLOW_ATTR_MASK is also
missing -- may be good to fix that also.
in flow_netlink.c for function ovs_nla_get_flow_metadata and
ovs_nla_get_match. It may be good to add comment about the newly added
argument.
in ovs_nla_get_match() in the mask section of parsing
OVS_KEY_ATTR_ENCAP, the parse_flow_mask_nlattrs(encap ...) call is
longer than 80 characters.
On Wed, Sep 10, 2014 at 2:06 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
> This new flag is useful for suppressing error logging when error
> messages may be expected, as when probing for datapath features using
> flow commands.
>
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
> ---
> datapath/datapath.c | 26 +++++++++------
> datapath/datapath.h | 6 ++++
> datapath/flow.c | 4 +--
> datapath/flow.h | 2 +-
> datapath/flow_netlink.c | 36
> ++++++++++++---------
> datapath/flow_netlink.h | 8 ++---
> datapath/linux/compat/include/linux/openvswitch.h | 3 ++
> 7 files changed, 52 insertions(+), 33 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 59f73d7..30e082d 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -525,6 +525,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb,
> struct genl_info *info)
> struct vport *input_vport;
> int len;
> int err;
> + bool log = !a[OVS_FLOW_ATTR_SILENT];
>
> err = -EINVAL;
> if (!a[OVS_PACKET_ATTR_PACKET] || !a[OVS_PACKET_ATTR_KEY] ||
> @@ -558,7 +559,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb,
> struct genl_info *info)
> goto err_kfree_skb;
>
> err = ovs_flow_key_extract_userspace(a[OVS_PACKET_ATTR_KEY], packet,
> - &flow->key);
> + &flow->key, log);
> if (err)
> goto err_flow_free;
>
> @@ -855,15 +856,16 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct
> genl_info *info)
> struct sw_flow_actions *acts;
> struct sw_flow_match match;
> int error;
> + bool log = !a[OVS_FLOW_ATTR_SILENT];
>
> /* Must have key and actions. */
> error = -EINVAL;
> if (!a[OVS_FLOW_ATTR_KEY]) {
> - OVS_NLERR("Flow key attribute not present in new flow.\n");
> + OVS_NLERR_IF(log, "Flow key attribute not present in new
> flow.\n");
> goto error;
> }
> if (!a[OVS_FLOW_ATTR_ACTIONS]) {
> - OVS_NLERR("Flow actions attribute not present in new
> flow.\n");
> + OVS_NLERR_IF(log, "Flow actions attribute not present in new
> flow.\n");
> goto error;
> }
>
> @@ -877,8 +879,8 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct
> genl_info *info)
>
> /* Extract key. */
> ovs_match_init(&match, &new_flow->unmasked_key, &mask);
> - error = ovs_nla_get_match(&match,
> - a[OVS_FLOW_ATTR_KEY],
> a[OVS_FLOW_ATTR_MASK]);
> + error = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY],
> + a[OVS_FLOW_ATTR_MASK], log);
> if (error)
> goto err_kfree_flow;
>
> @@ -888,7 +890,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct
> genl_info *info)
> error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS], &new_flow->key,
> &acts);
> if (error) {
> - OVS_NLERR("Flow actions may not be safe on all matching
> packets.\n");
> + OVS_NLERR_IF(log, "Flow actions may not be safe on all
> matching packets.\n");
> goto err_kfree_acts;
> }
>
> @@ -1012,6 +1014,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct
> genl_info *info)
> struct sw_flow_actions *old_acts = NULL, *acts = NULL;
> struct sw_flow_match match;
> int error;
> + bool log = !a[OVS_FLOW_ATTR_SILENT];
>
> /* Extract key. */
> error = -EINVAL;
> @@ -1021,8 +1024,8 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct
> genl_info *info)
> }
>
> ovs_match_init(&match, &key, &mask);
> - error = ovs_nla_get_match(&match,
> - a[OVS_FLOW_ATTR_KEY],
> a[OVS_FLOW_ATTR_MASK]);
> + error = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY],
> + a[OVS_FLOW_ATTR_MASK], log);
> if (error)
> goto error;
>
> @@ -1109,6 +1112,7 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct
> genl_info *info)
> struct datapath *dp;
> struct sw_flow_match match;
> int err;
> + bool log = !a[OVS_FLOW_ATTR_SILENT];
>
> if (!a[OVS_FLOW_ATTR_KEY]) {
> OVS_NLERR("Flow get message rejected, Key attribute
> missing.\n");
> @@ -1116,7 +1120,7 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct
> genl_info *info)
> }
>
> ovs_match_init(&match, &key, NULL);
> - err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL);
> + err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL, log);
> if (err)
> return err;
>
> @@ -1157,10 +1161,12 @@ static int ovs_flow_cmd_del(struct sk_buff *skb,
> struct genl_info *info)
> struct datapath *dp;
> struct sw_flow_match match;
> int err;
> + bool log = !a[OVS_FLOW_ATTR_SILENT];
>
> if (likely(a[OVS_FLOW_ATTR_KEY])) {
> ovs_match_init(&match, &key, NULL);
> - err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL);
> + err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL,
> + log);
> if (unlikely(err))
> return err;
> }
> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index c5d3c86..1b8db14 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
> @@ -209,4 +209,10 @@ do {
> \
> if (net_ratelimit()) \
> pr_info("netlink: " fmt, ##__VA_ARGS__); \
> } while (0)
> +
> +#define OVS_NLERR_IF(logging_allowed, fmt, ...) \
> +do { \
> + if (logging_allowed) \
> + OVS_NLERR(fmt, ##__VA_ARGS__); \
> +} while (0)
> #endif /* datapath.h */
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 12ebc5b..84a85c4 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -712,12 +712,12 @@ int ovs_flow_key_extract(const struct ovs_tunnel_info
> *tun_info,
>
> int ovs_flow_key_extract_userspace(const struct nlattr *attr,
> struct sk_buff *skb,
> - struct sw_flow_key *key)
> + struct sw_flow_key *key, bool log)
> {
> int err;
>
> /* Extract metadata from netlink attributes. */
> - err = ovs_nla_get_flow_metadata(attr, key);
> + err = ovs_nla_get_flow_metadata(attr, key, log);
> if (err)
> return err;
>
> diff --git a/datapath/flow.h b/datapath/flow.h
> index 44ed10d..b25ab87 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -257,7 +257,7 @@ int ovs_flow_key_extract(const struct ovs_tunnel_info
> *tun_info,
> /* Extract key from packet coming from userspace. */
> int ovs_flow_key_extract_userspace(const struct nlattr *attr,
> struct sk_buff *skb,
> - struct sw_flow_key *key);
> + struct sw_flow_key *key, bool log);
> /* Update the non-metadata part of the flow key using skb. */
> int ovs_flow_key_update(struct sk_buff *skb, struct sw_flow_key *key);
>
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 6c74841..5c24376 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -327,7 +327,7 @@ static bool is_all_zero(const u8 *fp, size_t size)
>
> static int __parse_flow_nlattrs(const struct nlattr *attr,
> const struct nlattr *a[],
> - u64 *attrsp, bool nz)
> + u64 *attrsp, bool log, bool nz)
> {
> const struct nlattr *nla;
> u64 attrs;
> @@ -339,19 +339,19 @@ static int __parse_flow_nlattrs(const struct nlattr
> *attr,
> int expected_len;
>
> if (type > OVS_KEY_ATTR_MAX) {
> - OVS_NLERR("Unknown key attribute (type=%d,
> max=%d).\n",
> + OVS_NLERR_IF(log, "Unknown key attribute (type=%d,
> max=%d).\n",
> type, OVS_KEY_ATTR_MAX);
> return -EINVAL;
> }
>
> if (attrs & (1ULL << type)) {
> - OVS_NLERR("Duplicate key attribute (type %d).\n",
> type);
> + OVS_NLERR_IF(log, "Duplicate key attribute (type
> %d).\n", type);
> return -EINVAL;
> }
>
> expected_len = ovs_key_lens[type];
> if (nla_len(nla) != expected_len && expected_len != -1) {
> - OVS_NLERR("Key attribute has unexpected length
> (type=%d"
> + OVS_NLERR_IF(log, "Key attribute has unexpected
> length (type=%d"
> ", length=%d, expected=%d).\n", type,
> nla_len(nla), expected_len);
> return -EINVAL;
> @@ -363,7 +363,7 @@ static int __parse_flow_nlattrs(const struct nlattr *attr,
> }
> }
> if (rem) {
> - OVS_NLERR("Message has %d unknown bytes.\n", rem);
> + OVS_NLERR_IF(log, "Message has %d unknown bytes.\n", rem);
> return -EINVAL;
> }
>
> @@ -372,15 +372,17 @@ static int __parse_flow_nlattrs(const struct nlattr
> *attr,
> }
>
> static int parse_flow_mask_nlattrs(const struct nlattr *attr,
> - const struct nlattr *a[], u64 *attrsp)
> + const struct nlattr *a[], u64 *attrsp,
> + bool log)
> {
> - return __parse_flow_nlattrs(attr, a, attrsp, true);
> + return __parse_flow_nlattrs(attr, a, attrsp, log, true);
> }
>
> static int parse_flow_nlattrs(const struct nlattr *attr,
> - const struct nlattr *a[], u64 *attrsp)
> + const struct nlattr *a[], u64 *attrsp,
> + bool log)
> {
> - return __parse_flow_nlattrs(attr, a, attrsp, false);
> + return __parse_flow_nlattrs(attr, a, attrsp, log, false);
> }
>
> static int ipv4_tun_from_nlattr(const struct nlattr *attr,
> @@ -932,7 +934,8 @@ static void mask_set_nlattr(struct nlattr *attr, u8 val)
> */
> int ovs_nla_get_match(struct sw_flow_match *match,
> const struct nlattr *nla_key,
> - const struct nlattr *nla_mask)
> + const struct nlattr *nla_mask,
> + bool log)
> {
> const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];
> const struct nlattr *encap;
> @@ -942,7 +945,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
> bool encap_valid = false;
> int err;
>
> - err = parse_flow_nlattrs(nla_key, a, &key_attrs);
> + err = parse_flow_nlattrs(nla_key, a, &key_attrs, log);
> if (err)
> return err;
>
> @@ -964,7 +967,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
> encap_valid = true;
>
> if (tci & htons(VLAN_TAG_PRESENT)) {
> - err = parse_flow_nlattrs(encap, a, &key_attrs);
> + err = parse_flow_nlattrs(encap, a, &key_attrs, log);
> if (err)
> return err;
> } else if (!tci) {
> @@ -1014,7 +1017,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
> nla_mask = newmask;
> }
>
> - err = parse_flow_mask_nlattrs(nla_mask, a, &mask_attrs);
> + err = parse_flow_mask_nlattrs(nla_mask, a, &mask_attrs, log);
> if (err)
> goto free_newmask;
>
> @@ -1038,7 +1041,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
> if (eth_type == htons(0xffff)) {
> mask_attrs &= ~(1ULL <<
> OVS_KEY_ATTR_ETHERTYPE);
> encap = a[OVS_KEY_ATTR_ENCAP];
> - err = parse_flow_mask_nlattrs(encap, a,
> &mask_attrs);
> + err = parse_flow_mask_nlattrs(encap, a,
> &mask_attrs, log);
> if (err)
> goto free_newmask;
> } else {
> @@ -1083,14 +1086,15 @@ free_newmask:
> * extracted from the packet itself.
> */
> int ovs_nla_get_flow_metadata(const struct nlattr *attr,
> - struct sw_flow_key *key)
> + struct sw_flow_key *key,
> + bool log)
> {
> const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];
> struct sw_flow_match match;
> u64 attrs = 0;
> int err;
>
> - err = parse_flow_nlattrs(attr, a, &attrs);
> + err = parse_flow_nlattrs(attr, a, &attrs, log);
> if (err)
> return -EINVAL;
>
> diff --git a/datapath/flow_netlink.h b/datapath/flow_netlink.h
> index 90bbe37..2cf15c2 100644
> --- a/datapath/flow_netlink.h
> +++ b/datapath/flow_netlink.h
> @@ -45,11 +45,11 @@ void ovs_match_init(struct sw_flow_match *match,
>
> int ovs_nla_put_flow(const struct sw_flow_key *,
> const struct sw_flow_key *, struct sk_buff *);
> -int ovs_nla_get_flow_metadata(const struct nlattr *, struct sw_flow_key *);
> +int ovs_nla_get_flow_metadata(const struct nlattr *, struct sw_flow_key *,
> + bool log);
>
> -int ovs_nla_get_match(struct sw_flow_match *match,
> - const struct nlattr *,
> - const struct nlattr *);
> +int ovs_nla_get_match(struct sw_flow_match *, const struct nlattr *key,
> + const struct nlattr *mask, bool log);
> int ovs_nla_put_egress_tunnel_key(struct sk_buff *,
> const struct ovs_tunnel_info *);
>
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h
> b/datapath/linux/compat/include/linux/openvswitch.h
> index 306ea86..14c05fc 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -483,6 +483,9 @@ enum ovs_flow_attr {
> OVS_FLOW_ATTR_USED, /* u64 msecs last used in monotonic time. */
> OVS_FLOW_ATTR_CLEAR, /* Flag to clear stats, tcp_flags, used. */
> OVS_FLOW_ATTR_MASK, /* Sequence of OVS_KEY_ATTR_* attributes. */
> + OVS_FLOW_ATTR_SILENT, /* Flag to suppress any logging for the flow
> + * operation, useful when probing for feature
> + * compatibility. */
> __OVS_FLOW_ATTR_MAX
> };
>
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
