[ovs-dev] [PATCH 5/9] Native OpenFlow 1.2 set_field action support.

[Ben Pfaff]

On Thu, Oct 17, 2013 at 12:37:27PM -0700, Ben Pfaff wrote:
> On Wed, Oct 16, 2013 at 04:16:07PM -0700, Jarno Rajahalme wrote:
> > This is a little bit simpler and marginally more efficient, but also makes
> > following changes easier.
> > 
> > Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> I had to read the patch before I understood what it did.  Could you
> elaborate the change log slightly?
> 
> Any particular reason to inline mf_from_id()?  I don't really object, I
> think, but rationale would be nice.  Also I don't see yet how this is
> related to the rest of the patch.  Could it be separated?
> 
> Because ofpacts are padded to a multiple of 8 bytes, and union mf_value
> is 16 bytes long, it looks to me like the trickery around struct
> ofpact_set_field could save at most 8 bytes, so that a struct
> ofpact_set_field is either 16 bytes or 24 bytes long when included in a
> series of struct ofpacts.  I doubt that's worth it.

Also: I'd be inclined to put the functionality of
ofputil_set_field_to_openflow() inside ofp-actions.c.  Any particular
reason this adds it to ofp-util.c?