On 20 March 2018 at 13:46, Ben Pfaff <[email protected]> wrote: > Until mow, this macro has blindly read the passed-in type's size, but > that's unnecessarily risky. This commit changes it to verify that the > passed-in type is the same size as the field and, on GCC and Clang, that > the types are compatible. It also adds a version that does not check, > for the one case where (currently) we deliberately read the wrong size, > and updates a few uses to use more precise field names. > > Signed-off-by: Ben Pfaff <[email protected]> > --- > lib/classifier.c | 8 ++++---- > lib/dpif-netdev.c | 3 ++- > lib/flow.c | 2 +- > lib/flow.h | 17 ++++++++++++++++- > 4 files changed, 23 insertions(+), 7 deletions(-) > > diff --git a/lib/classifier.c b/lib/classifier.c > index ef7e3de0728c..cea9053b6f67 100644 > --- a/lib/classifier.c > +++ b/lib/classifier.c > @@ -495,8 +495,8 @@ classifier_count(const struct classifier *cls) > static inline ovs_be32 minimatch_get_ports(const struct minimatch *match) > { > /* Could optimize to use the same map if needed for fast path. */ > - return MINIFLOW_GET_BE32(match->flow, tp_src) > - & MINIFLOW_GET_BE32(&match->mask->masks, tp_src); > + return (miniflow_get_ports(match->flow) > + & miniflow_get_ports(&match->mask->masks)); > } > > /* Inserts 'rule' into 'cls' in 'version'. Until 'rule' is removed from > 'cls', > @@ -1501,7 +1501,7 @@ insert_subtable(struct classifier *cls, const struct > minimask *mask) > /* Ports trie. */ > ovsrcu_set_hidden(&subtable->ports_trie, NULL); > *CONST_CAST(int *, &subtable->ports_mask_len) > - = 32 - ctz32(ntohl(MINIFLOW_GET_BE32(&mask->masks, tp_src))); > + = 32 - ctz32(ntohl(miniflow_get_ports(&mask->masks))); > > /* List of rules. */ > rculist_init(&subtable->rules_list); > @@ -1696,7 +1696,7 @@ find_match_wc(const struct cls_subtable *subtable, > ovs_version_t version, > unsigned int mbits; > ovs_be32 value, plens, mask; > > - mask = MINIFLOW_GET_BE32(&subtable->mask.masks, tp_src); > + mask = miniflow_get_ports(&subtable->mask.masks); > value = ((OVS_FORCE ovs_be32 *)flow)[TP_PORTS_OFS32] & mask; > mbits = trie_lookup_value(&subtable->ports_trie, &value, &plens, > 32); > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index b07fc6b8b327..be31fd0922b0 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -2200,7 +2200,8 @@ dp_netdev_pmd_lookup_flow(struct > dp_netdev_pmd_thread *pmd, > { > struct dpcls *cls; > struct dpcls_rule *rule; > - odp_port_t in_port = u32_to_odp(MINIFLOW_GET_U32(&key->mf, in_port)); > + odp_port_t in_port = u32_to_odp(MINIFLOW_GET_U32(&key->mf, > + in_port.odp_port)); > struct dp_netdev_flow *netdev_flow = NULL; > > cls = dp_netdev_pmd_lookup_dpcls(pmd, in_port); > diff --git a/lib/flow.c b/lib/flow.c > index 38ff29c8cd14..09b66b81858f 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -2007,7 +2007,7 @@ miniflow_hash_5tuple(const struct miniflow *flow, > uint32_t basis) > } > > /* Add both ports at once. */ > - hash = hash_add(hash, MINIFLOW_GET_U32(flow, tp_src)); > + hash = hash_add(hash, (OVS_FORCE uint32_t) > miniflow_get_ports(flow)); > } > out: > return hash_finish(hash, 42); > diff --git a/lib/flow.h b/lib/flow.h > index 770a07a62778..3331e2068ed8 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -684,6 +684,14 @@ miniflow_get__(const struct miniflow *mf, size_t idx) > /* Get the value of the struct flow 'FIELD' as up to 8 byte wide integer > type > * 'TYPE' from miniflow 'MF'. */ > #define MINIFLOW_GET_TYPE(MF, TYPE, FIELD) \ > + (BUILD_ASSERT(sizeof(TYPE) == sizeof(((struct flow *)0)->FIELD)), \ > + BUILD_ASSERT_GCCONLY(__builtin_types_compatible_p(TYPE, > typeof(((struct flow *)0)->FIELD))), \ > + MINIFLOW_GET_TYPE__(MF, TYPE, FIELD)) > + > +/* Like MINIFLOW_GET_TYPE, but without checking that TYPE is the correct > width > + * for FIELD. (This is useful for deliberately reading adjacent fields > in one > + * go.) */ > +#define MINIFLOW_GET_TYPE__(MF, TYPE, FIELD) \ > (MINIFLOW_IN_MAP(MF, FLOW_U64_OFFSET(FIELD)) \ > ? ((OVS_FORCE const TYPE *)miniflow_get__(MF, > FLOW_U64_OFFSET(FIELD))) \ > [FLOW_U64_OFFREM(FIELD) / sizeof(TYPE)] \ > @@ -806,7 +814,7 @@ miniflow_get_vid(const struct miniflow *flow, size_t n) > { > if (n < FLOW_MAX_VLAN_HEADERS) { > union flow_vlan_hdr hdr = { > - .qtag = MINIFLOW_GET_BE32(flow, vlans[n]) > + .qtag = MINIFLOW_GET_BE32(flow, vlans[n].qtag) > }; > return vlan_tci_to_vid(hdr.tci); > } > @@ -849,6 +857,13 @@ miniflow_get_metadata(const struct miniflow *flow) > return MINIFLOW_GET_BE64(flow, metadata); > } > > + > +/* Returns the 'tp_src' and 'tp_dst' fields together as one piece of > data. */ > +static inline ovs_be32 > +miniflow_get_ports(const struct miniflow *flow) > +{ > + return MINIFLOW_GET_TYPE__(flow, ovs_be32, tp_src); > +} > /* Returns the mask for the OpenFlow 1.1+ "metadata" field in 'mask'. > * > * The return value is all-1-bits if 'mask' matches on the whole value of > the > -- > 2.16.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
LGTM. Reviewed-by: Armando Migliaccio <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
