Hey Ben,
Thanks for the reviews. I am posting a new patch addressing your concerns.
thanx!
mehak
On Thu, Aug 16, 2012 at 10:44 AM, Ben Pfaff <b...@nicira.com> wrote:
> 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.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
