I think in general this patch would be cleaner if we required the void *
parameters to be properly aligned. I don't think that should block it though,
we can always change it later.
I'm also wondering if it's possible to generate a lot of this code. At any
rate, this is a step in the right direction.
I think mf_format() could use a NOT_REACHED in the default case just like
mf_parse() it also has an extra newline at the end of the function.
+ {
+ MFF_TUN_ID, "tun_id", NULL,
+ 8, 64,
+ MFM_FULLY, 0,
+ MFS_HEXADECIMAL,
+ MFP_NONE,
+ NXM_NX_TUN_ID,
+ }, {
In this array I would user sizeof for they byte counts and sizeof * 8 for the
bit counts except in the special cases. Seems less error prone.
+/* Not yet implemented. */
+bool mf_is_all_fixed(const struct mf_field *, const struct flow_wildcards *);
+int mf_n_wild_bits(const struct mf_field *, const struct flow_wildcards *);
+int mf_n_fixed_bits(const struct mf_field *, const struct flow_wildcards *);
Is there a good reason to keep these in the patch?
+/* Initializes the 'mf->n_bytes' in 'mask' with the wildcard bit pattern for
+ * field 'mf' within 'wc'. Each bit in 'mask' will be set to 1 if the bit is
+ * significant for matching purposes, or to 0 if it is wildcarded.
+ *
+ * The caller is responsible for ensuring that 'wc' corresponds to a flow that
+ * meets 'mf''s prerequisites. */
It might be worth noting that 'mask' is interpreted in network byte order in
this comment.
+ case MFF_VLAN_VID:
+ /* XXX? */
+ put_unaligned_be16(value, flow->vlan_tci & htons(VLAN_VID_MASK));
I think this XXX could use an explanation. It's not clear to me exactly what
it's worried about.
+ switch (mf->id) {
+ case MFF_IN_PORT:
+ case MFF_ETH_SRC:
+ case MFF_ETH_TYPE:
+ case MFF_VLAN_VID:
+ case MFF_VLAN_PCP:
+ case MFF_IP_PROTO:
+ case MFF_IP_TOS:
+ case MFF_ARP_OP:
+ case MFF_ARP_SHA:
+ case MFF_ARP_THA:
+ case MFF_TCP_SRC:
+ case MFF_TCP_DST:
+ case MFF_UDP_SRC:
+ case MFF_UDP_DST:
+ case MFF_ICMP_TYPE:
+ case MFF_ICMP_CODE:
+ case MFF_ND_TARGET:
+ case MFF_ND_SLL:
+ case MFF_ND_TLL:
+ /* not valid */
+ break;
Would it be appropriate to NOT_REACHED here?
+ mf_set(mf, value, mask, rule);
+ }
+}
+
+
+/* Copies the value and wildcard bit pattern for 'mf' from 'rule' into the
+ * 'mf->n_bytes' bytes in each of 'value' and 'mask'. */
+void
+mf_get(const struct mf_field *mf, const struct cls_rule *rule,
+ void *value, void *mask)
+{
Redundant newline above this function.
+ case MFF_VLAN_VID:
+ *(ovs_be16 *) value &= htons(VLAN_VID_MASK); /* XXX */
+ break;
I suppose this XXX could use an explanation as well.
+/* Prerequisites for matching a field.
+ *
+ * A field may only be matched if the correct lower-level protocols are also
+ * matched. For example, the TCP port may be matched only if the Ethernet type
+ * matches ETH_TYPE_IP and the IP protocol matches IPPROTO_TCP. */
+enum mf_prereqs {
+ MFP_NONE,
+
+ /* L2 prerequisites. */
+ MFP_ARP,
+ MFP_IPV4,
+ MFP_IPV6,
+ MFP_IP_ANY,
+
+ /* L3 prerequisites. */
+ MFP_TCP, /* On IPv4 or IPv6. */
+ MFP_UDP, /* On IPv4 or IPv6. */
+ MFP_ICMPV6,
+ MFP_ICMP_ANY,
+
+ /* L4 prerequisites. */
+ MFP_ND,
+ MFP_ND_SOLICIT,
+ MFP_ND_ADVERT
+};
Perhaps I'm confused about the /* L[234] prerequisites */ comments, but
shouldn't they each be shifted up one? MFP_IPV4 is an L3 not an L2 field for
example.
+ MFM_MCAST /* Byte 0, bit 2**0 is separately maskable. */
Do you mean Byte 0, bit 0?
Ethan
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
