[ovs-dev] [PATCH 2/5] lib/ofp-actions: helper functions for of12 set-field action

Simon Horman horms at verge.net.au
Thu Sep 13 01:33:35 UTC 2012


On Wed, Sep 12, 2012 at 11:13:49AM -0700, Ben Pfaff wrote:
> On Wed, Sep 12, 2012 at 05:44:29PM +0900, Simon Horman wrote:
> > +enum ofperr
> > +nxm_reg_load_from_openflow12_set_field(
> > +    const struct ofp12_action_set_field * oasf, struct ofpbuf *ofpacts)
> > +{
> > +    uint16_t oasf_len = ntohs(oasf->len);
> > +    ovs_be32 *p = (ovs_be32*)oasf->field;
> > +    uint32_t oxm_header = ntohl(*p);
> > +    uint8_t oxm_length = NXM_LENGTH(oxm_header);
> > +    struct ofpact_reg_load *load;
> > +    const struct mf_field *mf;
> > +    int i;
> > +
> > +    /* ofp12_action_set_field is padded to 64 bits by zero */
> > +    if (oasf_len != ROUND_UP(sizeof(*oasf) + oxm_length, 8)) {
> > +        return OFPERR_OFPBAC_BAD_ARGUMENT;
> 
> This seems to be just coincidentally correct, simply because
> oasf->field[] happens to be the right length.
> 
> I think that we should change struct ofp12_action_set_field so that it
> ends with "ovs_be32 dst;" instead of "uint8_t field[4];" (and update
> the comments).  Then it would make more sense to do the arithmetic
> here, and we could skip the silliness with the cast to ovs_be32 above.

I'm very much in favour of avoiding casts.
I'll see about implementing your suggestion.

> > +    }
> > +    for (i = sizeof(*oasf) + oxm_length; i < oasf_len; i++) {
> > +        if (((const char*)oasf)[i]) {
> > +            return OFPERR_OFPBAC_BAD_ARGUMENT;
> > +        }
> > +    }
> 
> Please use is_all_zeros() instead of the loop above.

Thanks, will do.

> > +    if (NXM_HASMASK(oxm_header)) {
> > +        return OFPERR_OFPBAC_BAD_ARGUMENT;
> > +    }
> > +    mf = mf_from_nxm_header(oxm_header);
> > +    if (!mf || mf->oxm_header == 0) {
> 
> I guess mf->oxm_header == 0 is meant to rule out non-OXM fields.  But
> I don't think that's a good idea.  Extensibility is an OXM/NXM design
> goal and I don't know why we'd rule out non-official OXM fields.

Thanks, I'll fix that.

> > +        return OFPERR_OFPBAC_BAD_ARGUMENT;
> > +    }
> 
> >  enum ofperr
> >  nxm_reg_load_check(const struct ofpact_reg_load *load, const struct flow *flow)
> >  {
> > +    const struct mf_field *mf;
> > +    union mf_value value;
> > +
> > +    if (load->ofpact.compat != OFPUTIL_OFPAT12_SET_FIELD) {
> > +        return mf_check_dst(&load->dst, flow);
> 
> I think it would be good to do the same checks for set-field as we do
> for reg-load.  Do you know a good reason why not?  The kinds of checks
> we do are pretty minimal, just to rule out values that don't make
> sense at all for a given field.
> 
> If there's really some reason why we can't do those checks for
> set-field, then I'd prefer to do the mf_check_dst() check up front
> here, in one place, for both set-field and reg-load.

I think it just comes down to how liberal the parser is desired to be.
I'll implement your suggestion and see how it goes.

> >  {
> >      struct nx_action_reg_load *narl;
> >  
> > +    if (load->ofpact.compat == OFPUTIL_OFPAT12_SET_FIELD) {
> > +        const struct mf_field *mf = load->dst.field;
> > +        struct ofp12_action_set_field *oasf;
> > +
> > +        oasf = ofputil_put_OFPAT12_SET_FIELD(openflow);
> > +        *(uint32_t *)oasf->field = mf->oxm_header;
> > +        memset(oasf + 1, 0, mf->n_bytes);
> > +        bitwise_copy(&load->subvalue, sizeof load->subvalue, load->dst.ofs,
> > +                     oasf + 1, mf->n_bytes, load->dst.ofs, load->dst.n_bits);
> 
> Hmm.  This looks like it blindly puts an OFPAT12_SET_FIELD action into
> other versions of OpenFlow.  I think we have a few options:

True, but I'm not sure that I understand how it could legitimately occur.

>         * Don't worry about it.  That might be OK because
>           OFPAT12_SET_FIELD doesn't collide with any assigned OpenFlow
>           1.0 action (accidentally) or 1.1 action (by design).
> 
>         * Translate into a series of NXAST_REG_LOAD actions.

	    This is nice, but I'm not sure it is worth the complexity.

>         * Any other ideas?

	    How about, detect and return an error?



More information about the dev mailing list