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?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
