[ovs-dev] [of1.1 rollup 16/20] ofp-actions: Support decoding OpenFlow 1.1 instructions and actions.

[Simon Horman]

On Tue, Jun 12, 2012 at 12:32:20AM -0700, Ben Pfaff wrote:
> This builds and doesn't break any existing unit tests, but nothing
> calls it yet (and it doesn't have any unit tests of its own) so it
> can hardly be called useful.

[snip]

> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c

[snip]

> +enum ofperr
> +ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
> +                                     unsigned int instructions_len,
> +                                     struct ofpbuf *ofpacts)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +    const struct ofp11_instruction *instructions;
> +    const struct ofp11_instruction *insts[N_OVS_INSTRUCTIONS];
> +    enum ofperr error;
> +
> +    ofpbuf_clear(ofpacts);
> +
> +    if (instructions_len % OFP11_INSTRUCTION_ALIGN != 0) {
> +        VLOG_WARN_RL(&rl, "OpenFlow message instructions length %u is not a "
> +                     "multiple of %d",
> +                     instructions_len, OFP11_INSTRUCTION_ALIGN);
> +        error = OFPERR_OFPBRC_BAD_LEN;
> +        goto exit;
> +    }
> +
> +    instructions = ofpbuf_try_pull(openflow, instructions_len);
> +    if (instructions == NULL) {
> +        VLOG_WARN_RL(&rl, "OpenFlow message instructions length %u exceeds "
> +                     "remaining message length (%zu)",
> +                     instructions_len, openflow->size);
> +        error = OFPERR_OFPBRC_BAD_LEN;
> +        goto exit;
> +    }
> +
> +    error = decode_openflow11_instructions(
> +        instructions, instructions_len / OFP11_INSTRUCTION_ALIGN,
> +        insts);
> +    if (error) {
> +        goto exit;
> +    }
> +
> +    if (insts[OVSINST_OFPIT11_APPLY_ACTIONS]) {
> +        const union ofp_action *actions;
> +        size_t n_actions;
> +
> +        get_actions_from_instruction(insts[OVSINST_OFPIT11_APPLY_ACTIONS],
> +                                     &actions, &n_actions);
> +        error = ofpacts_from_openflow11(actions, n_actions, ofpacts);
> +        if (error) {
> +            goto exit;
> +        }
> +    }
> +
> +    ofpact_put_END(ofpacts);
> +
> +    if (insts[OVSINST_OFPIT11_GOTO_TABLE] ||
> +        insts[OVSINST_OFPIT11_WRITE_METADATA] ||
> +        insts[OVSINST_OFPIT11_WRITE_ACTIONS] ||
> +        insts[OVSINST_OFPIT11_CLEAR_ACTIONS]) {
> +        error = OFPERR_OFPBIC_UNSUP_INST;
> +        goto exit;
> +    }
> +
> +exit:
>      if (error) {
>          ofpbuf_clear(ofpacts);
>      }

The next and last line of ofpacts_pull_openflow11_instructions() ends up
being:

    return 0;

I wonder if it should be:

    return error;

[snip]