Thanks for the review. The changes will be rolled into V6 of the patch, to
be sent out soon.
On Tue, Jun 18, 2013 at 2:48 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Mon, Jun 17, 2013 at 07:51:01AM -0700, Andy Zhou wrote:
> > Added support to allow mega flow specified and displayed. ovs-dpctl tool
> > is mainly used as debugging tool.
> >
> > This patch also implements the low level user space routines to send
> > and receive mega flow netlink messages. Those netlink suppor
> > routines are required for forthcoming user space mega flow patches.
> >
> > Added a unit test to test parsing and display of mega flows.
> >
> > Ethan contributed the ovs-dpctl mega flow output function.
> >
> > Co-authored-by: Ethan Jackson <et...@nicira.com>
> > Signed-off-by: Ethan Jackson <et...@nicira.com>
> > Signed-off-by: Andy Zhou <az...@nicira.com>
>
> GCC complains:
>
> ../lib/odp-util.c: In function ‘parse_odp_key_mask_attr’:
> ../lib/odp-util.c:2151: error: ‘encap_mask’ may be used uninitialized
> in this function
> ../lib/odp-util.c: In function ‘format_odp_key_attr’:
> ../lib/odp-util.c:909: error: ‘is_exact’ may be used uninitialized in
> this function
>
>
> In lib/dpif-linux.c, there is a missing space before the ':' in the
> conditional operator:
>
> @@ -939,10 +944,14 @@ dpif_linux_flow_dump_next(const struct dpif *dpif_
> OVS_UNUSED, void *state_,
> }
> if (key) {
> *key = state->flow.key;
> *key_len = state->flow.key_len;
> }
> + if (mask) {
> + *mask = state->flow.mask;
> + *mask_len = state->flow.mask ? state->flow.mask_len: 0;
> + }
>
> format_mpls() in odp-util.c has a style typo here:
> + }else {
>
> In format_odp_key_attr(), is_exact is left uninitialized if attr ==
> OVS_KEY_ATTR_TUNNEL but tun_mask.flags != (...long expression...).
>
> I think that an odp_mask_attr_is_exact() function is warranted and
> would make the code more readable. You'd end up with
> if (ma && odp_mask_attr_is_exact(ma)) {
> ma = NULL;
> }
> which is easy to understand.
>
> This code here in format_odp_key_attr() looks a little awkward to me:
> {
> bool bad_key_len, bad_mask_len;
> expected_len = odp_flow_key_attr_len(nl_attr_type(a));
> bad_key_len = (expected_len != -2 && nl_attr_get_size(a) !=
> expected_len);
>
> if (ma) {
> expected_len = odp_flow_key_attr_len(nl_attr_type(ma));
> bad_mask_len = (expected_len != -2 && nl_attr_get_size(ma) !=
> expected_len);
> } else {
> bad_mask_len = false;
> }
>
> How about this instead:
> expected_len = odp_flow_key_attr_len(nl_attr_type(a));
> if (expected_len != -2) {
> bool bad_key_len = nl_attr_get_size(a) != expected_len;
> bool bad_mask_len = ma && nl_attr_get_size(ma) != expected_len;
>
> I think that's all.
>
> Thanks,
>
> Ben.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
