[ovs-dev] [PATCH] Add Nicira vendor extension action NXAST_DEC_TTL_CNT_IDS.
Ben Pfaff
blp at nicira.com
Thu Aug 16 17:44:57 UTC 2012
This is getting really close.
On Wed, Aug 15, 2012 at 03:00:10PM -0700, Mehak Mahajan wrote:
> static enum ofperr
> +dec_ttl_from_openflow(struct ofpbuf *out)
> +{
> + uint16_t id = 0;
> + struct ofpact_cnt_ids *ids;
> + enum ofperr error = 0;
> +
> + ids = ofpact_put_DEC_TTL(out);
> + ids->ofpact.compat = OFPUTIL_NXAST_DEC_TTL;
> + ids->n_controllers = 1;
> + ids = out->l2;
I think you can delete the statement above because the value it
assigns to 'ids' is not used before 'ids' is assigned again below.
> + ofpbuf_put(out, &id, sizeof id);
> + ids = out->l2;
> + ofpact_update_len(out, &ids->ofpact);
> + return error;
> +}
> +
> +static enum ofperr
> +dec_ttl_cnt_ids_from_openflow(const struct nx_action_cnt_ids *nac_ids,
> + struct ofpbuf *out)
> +{
> + struct ofpact_cnt_ids *ids;
> + size_t ids_size;
> + enum ofperr error = 0;
'error' is never changed so I think you can remove it and just write
'return 0;' at the end of the function.
"sparse" pointed out that ntohl should be ntohs here:
> + ids_size = ntohl(nac_ids->len) - sizeof *nac_ids;
(If you do not use "sparse" routinely then I recommend it. See
INSTALL for instructions.)
...
> + return error;
> +}
> +
> +static enum ofperr
> decode_nxast_action(const union ofp_action *a, enum ofputil_action_code *code)
> {
> const struct nx_action_header *nah = (const struct nx_action_header *) a;
> @@ -310,7 +364,12 @@ ofpact_from_nxast(const union ofp_action *a, enum ofputil_action_code code,
> break;
>
> case OFPUTIL_NXAST_DEC_TTL:
> - ofpact_put_DEC_TTL(out);
> + error = dec_ttl_from_openflow(out);
> + break;
> +
> + case OFPUTIL_NXAST_DEC_TTL_CNT_IDS:
> + error = dec_ttl_cnt_ids_from_openflow(
> + (const struct nx_action_cnt_ids *) a, out);
> break;
>
> case OFPUTIL_NXAST_FIN_TIMEOUT:
> @@ -1083,6 +1142,30 @@ ofpact_controller_to_nxast(const struct ofpact_controller *oc,
> }
>
> static void
> +ofpact_dec_ttl_to_nxast(const struct ofpact_cnt_ids *oc_ids,
> + struct ofpbuf *out)
> +{
> + struct nx_action_cnt_ids *nac_ids;
> + int ids_len = ROUND_UP(2 * oc_ids->n_controllers, OFP_ACTION_ALIGN);
> + ovs_be16 *ids;
> + size_t i;
> + if (oc_ids->ofpact.compat == OFPUTIL_NXAST_DEC_TTL) {
> + ofputil_put_NXAST_DEC_TTL(out);
> + return;
> + } else {
> + nac_ids = ofputil_put_NXAST_DEC_TTL_CNT_IDS(out);
The formatting here is a little odd. I'd move the assignment above to
nac_ids to the top level instead of putting it into an 'else'. Or you
could put all the code below in the 'else', plus the declarations at
the top of the function, and then remove the 'return;' above.
> + }
> + nac_ids->len = htons(ntohs(nac_ids->len) + ids_len);
> + nac_ids->n_controllers = htons(oc_ids->n_controllers);
> +
> + ids = ofpbuf_put_zeros(out, ids_len);
> + for (i = 0; i < oc_ids->n_controllers; i++) {
> + ids[i] = htons(oc_ids->cnt_ids[i]);
> + }
> +}
> static void
> +print_dec_ttl(const struct ofpact_cnt_ids *ids,
> + struct ds *s)
> +{
> + size_t i;
> +
> + ds_put_cstr(s, "dec_ttl");
> + if (ids->ofpact.compat == OFPUTIL_NXAST_DEC_TTL_CNT_IDS) {
> + if (ids->n_controllers) {
I'm pretty sure that n_controllers always has to be nonzero, so why
the test above? (There's no corresponding test below for adding the
")".)
> + ds_put_cstr(s,"(");
There should be a space after the comma.
> + }
> + for (i = 0; i < ids->n_controllers; i++) {
> + if (i) {
> + ds_put_cstr(s, ",");
> + }
> + ds_put_format(s, "%"PRIu16, ids->cnt_ids[i]);
> + }
> + ds_put_cstr(s,")");
There should be a space after the comma.
> + }
> +}
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 32d3836..c726cd5 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -279,6 +279,62 @@ parse_controller(struct ofpbuf *b, char *arg)
> }
>
> static void
> +parse_dec_ttl(struct ofpbuf *b, char *arg)
> +{
> + struct ofpact_cnt_ids *ids;
> +
> + ids = ofpact_put_DEC_TTL(b);
> +
> + if (*arg == '\0') {
> + uint16_t id = 0;
> +
> + ids->ofpact.compat = OFPUTIL_NXAST_DEC_TTL;
> + ofpbuf_put(b, &id, sizeof id);
> + ids = b->l2;
> + ids->n_controllers++;
> + } else {
> + char *cntr;
> +
> + ids->ofpact.compat = OFPUTIL_NXAST_DEC_TTL_CNT_IDS;
> + cntr = strtok_r(arg, ", ", &arg);
> +
> + for (;;) {
> + uint16_t id;
> + char *p = cntr;
> +
> + if (!cntr) {
> + goto next;
> + }
> +
> + /* Verify that controller id is valid. */
> + while(*p != '\0') {
Missing space after 'while'.
> + if (!isdigit((unsigned char)*p)) {
Missing space before '*p'.
> + ovs_fatal(0, "dec_ttl: invalid controller id (%s)", cntr);
> + }
> + p++;
> + }
It's a nice thought to check for validity, but it probably isn't
necessary. We aren't that careful elsewhere, though perhaps someday
we will go through and make an attempt.
> + id = atoi(cntr);
> + ofpbuf_put(b, &id, sizeof id);
> + ids = b->l2;
> + ids->n_controllers++;
> +
> + next:
> + if (*arg == '\0') {
> + break;
> + }
> +
> + cntr = strtok_r(NULL, ", ", &arg);
> + }
This loop looks odd to me, especially the idea of testing '*arg'.
POSIX doesn't say exactly what 'arg' will point to, and although I
expect that most implementations use it the same way, I don't think
it's wise to rely on that, especially since it is not necessary.
Here is the way that I would write the loop (I didn't test it):
for (cntr = strtok_r(arg, ", ", &arg); cntr != NULL;
cntr = strtok_r(NULL, ", ", &arg)) {
uint16_t id = atoi(cntr);
ofpbuf_put(b, &id, sizeof id);
ids = b->l2;
ids->n_controllers++;
}
> + if (!ids->n_controllers) {
> + ovs_fatal(0, "dec_ttl_cnt_ids: expected at least one controller id.");
> + }
> +
> + }
> + ofpact_update_len(b, &ids->ofpact);
> +}
> +
> +static void
> parse_named_action(enum ofputil_action_code code, const struct flow *flow,
> char *arg, struct ofpbuf *ofpacts)
> {
> @@ -413,6 +469,7 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
>
> case OFPUTIL_NXAST_RESUBMIT_TABLE:
> case OFPUTIL_NXAST_OUTPUT_REG:
> + case OFPUTIL_NXAST_DEC_TTL_CNT_IDS:
> NOT_REACHED();
>
> case OFPUTIL_NXAST_LEARN:
> @@ -424,7 +481,7 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
> break;
>
> case OFPUTIL_NXAST_DEC_TTL:
> - ofpact_put_DEC_TTL(ofpacts);
> + parse_dec_ttl(ofpacts, arg);
> break;
>
> case OFPUTIL_NXAST_FIN_TIMEOUT:
> diff --git a/lib/ofp-util.def b/lib/ofp-util.def
> index 974cd8f..1d6d14b 100644
> --- a/lib/ofp-util.def
> +++ b/lib/ofp-util.def
> @@ -39,25 +39,26 @@ OFPAT11_ACTION(OFPAT11_SET_TP_DST, ofp_action_tp_port, "mod_tp_dst")
> #ifndef NXAST_ACTION
> #define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME)
> #endif
> -NXAST_ACTION(NXAST_RESUBMIT, nx_action_resubmit, 0, "resubmit")
> -NXAST_ACTION(NXAST_SET_TUNNEL, nx_action_set_tunnel, 0, "set_tunnel")
> -NXAST_ACTION(NXAST_SET_QUEUE, nx_action_set_queue, 0, "set_queue")
> -NXAST_ACTION(NXAST_POP_QUEUE, nx_action_pop_queue, 0, "pop_queue")
> -NXAST_ACTION(NXAST_REG_MOVE, nx_action_reg_move, 0, "move")
> -NXAST_ACTION(NXAST_REG_LOAD, nx_action_reg_load, 0, "load")
> -NXAST_ACTION(NXAST_NOTE, nx_action_note, 1, "note")
> -NXAST_ACTION(NXAST_SET_TUNNEL64, nx_action_set_tunnel64, 0, "set_tunnel64")
> -NXAST_ACTION(NXAST_MULTIPATH, nx_action_multipath, 0, "multipath")
> -NXAST_ACTION(NXAST_AUTOPATH, nx_action_autopath, 0, "autopath")
> -NXAST_ACTION(NXAST_BUNDLE, nx_action_bundle, 1, "bundle")
> -NXAST_ACTION(NXAST_BUNDLE_LOAD, nx_action_bundle, 1, "bundle_load")
> -NXAST_ACTION(NXAST_RESUBMIT_TABLE, nx_action_resubmit, 0, NULL)
> -NXAST_ACTION(NXAST_OUTPUT_REG, nx_action_output_reg, 0, NULL)
> -NXAST_ACTION(NXAST_LEARN, nx_action_learn, 1, "learn")
> -NXAST_ACTION(NXAST_EXIT, nx_action_header, 0, "exit")
> -NXAST_ACTION(NXAST_DEC_TTL, nx_action_header, 0, "dec_ttl")
> -NXAST_ACTION(NXAST_FIN_TIMEOUT, nx_action_fin_timeout, 0, "fin_timeout")
> -NXAST_ACTION(NXAST_CONTROLLER, nx_action_controller, 0, "controller")
> +NXAST_ACTION(NXAST_RESUBMIT, nx_action_resubmit, 0, "resubmit")
> +NXAST_ACTION(NXAST_SET_TUNNEL, nx_action_set_tunnel, 0, "set_tunnel")
> +NXAST_ACTION(NXAST_SET_QUEUE, nx_action_set_queue, 0, "set_queue")
> +NXAST_ACTION(NXAST_POP_QUEUE, nx_action_pop_queue, 0, "pop_queue")
> +NXAST_ACTION(NXAST_REG_MOVE, nx_action_reg_move, 0, "move")
> +NXAST_ACTION(NXAST_REG_LOAD, nx_action_reg_load, 0, "load")
> +NXAST_ACTION(NXAST_NOTE, nx_action_note, 1, "note")
> +NXAST_ACTION(NXAST_SET_TUNNEL64, nx_action_set_tunnel64, 0, "set_tunnel64")
> +NXAST_ACTION(NXAST_MULTIPATH, nx_action_multipath, 0, "multipath")
> +NXAST_ACTION(NXAST_AUTOPATH, nx_action_autopath, 0, "autopath")
> +NXAST_ACTION(NXAST_BUNDLE, nx_action_bundle, 1, "bundle")
> +NXAST_ACTION(NXAST_BUNDLE_LOAD, nx_action_bundle, 1, "bundle_load")
> +NXAST_ACTION(NXAST_RESUBMIT_TABLE, nx_action_resubmit, 0, NULL)
> +NXAST_ACTION(NXAST_OUTPUT_REG, nx_action_output_reg, 0, NULL)
> +NXAST_ACTION(NXAST_LEARN, nx_action_learn, 1, "learn")
> +NXAST_ACTION(NXAST_EXIT, nx_action_header, 0, "exit")
> +NXAST_ACTION(NXAST_DEC_TTL, nx_action_header, 0, "dec_ttl")
> +NXAST_ACTION(NXAST_DEC_TTL_CNT_IDS, nx_action_cnt_ids, 1, NULL)
> +NXAST_ACTION(NXAST_FIN_TIMEOUT, nx_action_fin_timeout, 0, "fin_timeout")
> +NXAST_ACTION(NXAST_CONTROLLER, nx_action_controller, 0, "controller")
>
> #undef OFPAT10_ACTION
> #undef OFPAT11_ACTION
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index cf34e92..20701e4 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5147,7 +5147,7 @@ execute_controller_action(struct action_xlate_ctx *ctx, int len,
> }
>
> static bool
> -compose_dec_ttl(struct action_xlate_ctx *ctx)
> +compose_dec_ttl(struct action_xlate_ctx *ctx, struct ofpact_cnt_ids *ids)
> {
> if (ctx->flow.dl_type != htons(ETH_TYPE_IP) &&
> ctx->flow.dl_type != htons(ETH_TYPE_IPV6)) {
> @@ -5158,7 +5158,12 @@ compose_dec_ttl(struct action_xlate_ctx *ctx)
> ctx->flow.nw_ttl--;
> return false;
> } else {
> - execute_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, 0);
> + size_t i;
> +
> + for (i = 0; i < ids->n_controllers; i++) {
> + execute_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL,
> + ids->cnt_ids[i]);
> + }
>
> /* Stop processing for current table. */
> return true;
> @@ -5518,7 +5523,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> break;
>
> case OFPACT_DEC_TTL:
> - if (compose_dec_ttl(ctx)) {
> + if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) {
> goto out;
> }
> break;
> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
> index ba8d309..dbe08ee 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -102,11 +102,14 @@ ffff 0010 00002320 0011 000000000000
> # actions=dec_ttl
> ffff 0010 00002320 0012 000000000000
>
> +# actions=dec_ttl(32768)
> +ffff 0018 00002320 0013 000100000000 8000000000000000
> +
> # actions=fin_timeout(idle_timeout=10,hard_timeout=20)
> -ffff 0010 00002320 0013 000a 0014 0000
> +ffff 0010 00002320 0014 000a 0014 0000
>
> # actions=controller(reason=invalid_ttl,max_len=1234,id=5678)
> -ffff 0010 00002320 0014 04d2 162e 02 00
> +ffff 0010 00002320 0015 04d2 162e 02 00
It looks like you renumbered some existing actions. If so, you can't
do that. You have to insert the new action after all of the actions
that have already been defined, to avoid confusing existing software.
Thanks,
Ben.
More information about the dev
mailing list