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.
> + }
> + 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.
> + 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.
> + 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.
> {
> 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:
* 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.
* Any other ideas?
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
