On Fri, May 10, 2013 at 10:54:36PM +0300, Jarno Rajahalme wrote:
> This helps get rid of one special case in nx_pull_raw() and allows
> loading of 32-bit values to OXM_OF_IN_PORT in NXAST_LEARN actions.
> Previously the 16-bit limit acted the same on both NXM_OF_IN_PORT and
> OXM_OF_IN_PORT, even though OF1.1+ controllers would expect OXM_OF_IN_PORT
> to be 32 bits wide.
Thanks, this seems very reasonable. I didn't know we had a bug in this
area. Thanks for finding and fixing it and adding a test. I think that
the patch can be improved, though, so some more comments follow.
I really like getting rid of the special case in nx_pull_raw().
> Signed-off-by: Jarno Rajahalme <jarno.rajaha...@nsn.com>
> ---
> I considered simply making the MFF_IN_PORT 32-bit instead, but that could
> have resulted in surprises for any existing use expecting it to be 16 bits
> (such as register load actions). Maybe there is no such conflict, and indeed
> it would be better to just widen the MFF_IN_PORT and add a special case for
> NXM_OF_IN_PORT into nx_pull_raw().
I think it's a bad idea to change the width of MFF_IN_PORT for exactly
the reason you mention. I don't know whether there is much (any?) code
that actually does a move or load of NXM_OF_IN_PORT, but I also don't
know of a good reason to break that code if there is.
This check is pretty close to the implementation of
ofputil_port_from_ofp11(). The only real difference is a log message.
I think it would be better to try to avoid duplicating code:
> + case MFF_IN_PORT_OXM: {
> + uint32_t port = ntohl(value->be32);
> + return port < OFPP_MAX || port >= OFPP11_MAX;
> + }
> +
> case MFF_IP_DSCP:
> return !(value->u8 & ~IP_DSCP_MASK);
> case MFF_IP_DSCP_SHIFTED:
The new port_from_ofp11() function is also very similar to
ofputil_port_from_ofp11(), so again I would prefer to avoid duplicating
code:
> +/* Map a 32 port number to 16-bit range. */
> +static inline uint16_t
> +port_from_ofp11(ovs_be32 ofp11_port)
> +{
> + uint32_t port = ntohl(ofp11_port);
> + return port < OFPP_MAX ? port
> + : port >= OFPP11_MAX ? port - OFPP11_OFFSET
> + : OFPP_NONE;
> +}
This code looks randomly indented and there is a missing {} around the
if's conditional statement. I have my doubts about the correctness of
the logic; perhaps it would be simpler to use ofputil_port_to_ofp11() on
a random 16-bit value?
> + case MFF_IN_PORT_OXM:
> + value->be32 &= htonl(0x0000ffff);
> + if (ntohl(value->be32) > OFPP_MAX)
> + value->be32 |= htonl(0xffff0000);
> + break;
> +
> case MFF_IPV6_LABEL:
> value->be32 &= ~htonl(IPV6_LABEL_MASK);
> break;
s/32_bit/32-bit/ here?
> + MFS_OFP_PORT_OXM, /* An OpenFlow port number or name (32_bit).
> */
> MFS_FRAG, /* no, yes, first, later, not_later */
> MFS_TNL_FLAGS, /* FLOW_TNL_F_* flags */
> };
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
