[ovs-dev] [PATCH] Add Nicira vendor extension action NXAST_DEC_TTL_CNT_IDS.

[Ben Pfaff]

On Wed, Aug 15, 2012 at 12:15:58AM -0700, Mehak Mahajan wrote:
> --- a/include/openflow/nicira-ext.h
> +++ b/include/openflow/nicira-ext.h
> @@ -291,7 +291,8 @@ enum nx_action_subtype {
>      NXAST_OUTPUT_REG,           /* struct nx_action_output_reg */
>      NXAST_LEARN,                /* struct nx_action_learn */
>      NXAST_EXIT,                 /* struct nx_action_header */
> -    NXAST_DEC_TTL,              /* struct nx_action_header */
> +    NXAST_DEC_TTL,              /* struct nx_action_cnt_ids */

It doesn't make sense to change NXAST_DEC_TTL.  Software already
exists that knows that NXAST_DEC_TTL is just an nx_action_header.  We
can't break that software.

> +    NXAST_DEC_TTL_CNT_IDS,      /* struct nx_action_cnt_ids */
>      NXAST_FIN_TIMEOUT,          /* struct nx_action_fin_timeout */
>      NXAST_CONTROLLER,           /* struct nx_action_controller */
>  };

...

> +
> + /* Action structure for NXAST_DEC_TTL and NXAST_DEC_TTL_CNT_IDS.
> +  *
> +  * The NXAST_DEC_TTL is used to forward the packet_in having reason code
> +  * OFPR_INVALID_TTL to all controllers having a zero controller id.  The
> +  * syntax for the same is `dec_ttl'.  The controller (having a zero id)
> +  * must have explicitly registered to receive these asynchronous messages.
> +  * Though the action does not explicitly have any controllers specified,
> +  * the n_controllers will be set to 1 and cnt_ids will contain controller
> +  * id 0.
> +  * The NXAST_DEC_TTL_CNT_IDS is used to forward the packet_in having reason
> +  * code OFPR_INVALID_TTL to a list of controllers.  The syntax for specifying
> +  * the same is `dec_ttl(id1, id2) where id1, id2 are valid controller ids
> +  * (please note that 0 is a valid controller id).  Similar to NXAST_DEC_TTL,
> +  * the controllers having the ids specified in the list must have explicitly
> +  * registered to receive these asynchronous messages.  The n_controllers will
> +  * be set to the number of controllers in the list and cnt_ids will contain
> +  * specified controller ids. */

This doesn't actually describe the main purpose of
NXAST_DEC_TTL_CNT_IDS, which is to decrement the IP TTL.  It only
describes what happens in exceptional cases, but it describes the
exceptional case as if it were the only case.

We don't usually describe the ovs-ofctl syntax for an action in the
comments in nicira-ext.h.  Conceptually, anyhow, nicira-ext.h is
supposed to be a header that anyone can copy out of OVS into their own
project that wants to use Nicira extensions, and what it's supposed to
contain is a complete protocol description for that use.

I suggest the following:

 /* Action structure for NXAST_DEC_TTL_CNT_IDS.
  *
  * If the packet is not IPv4 or IPv6, does nothing.  For IPv4 or IPv6, if the
  * TTL or hop limit is at least 2, decrements it by 1.  Otherwise, if TTL or
  * hop limit is 0 or 1, sends a packet-in to the controllers with each of the
  * 'n_controllers' controller IDs specified in 'cnt_ids'.
  *
  * (This differs from NXAST_DEC_TTL in that for NXAST_DEC_TTL the packet-in is
  * sent only to controllers with id 0.)
  */

> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -149,6 +149,66 @@ note_from_openflow(const struct nx_action_note *nan, struct ofpbuf *out)
>  }
>  
>  static enum ofperr
> +dec_ttl_from_openflow(const struct nx_action_cnt_ids *nac_ids,
> +                      struct ofpbuf *out)

This doesn't make sense because NXAST_DEC_TTL doesn't use struct
nx_action_cnt_ids.

> +{
...
> +}
> +
> +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;
> +    int i;
> +
> +    ids = ofpact_put_DEC_TTL(out);
> +    ids->ofpact.compat = OFPUTIL_NXAST_DEC_TTL_CNT_IDS;
> +    ids->n_controllers = ntohs(nac_ids->n_controllers);
> +    ids_size = ntohl(nac_ids->len) - sizeof *nac_ids;
> +
> +    if (!is_all_zeros(nac_ids->zeros, sizeof nac_ids->zeros)) {
> +        error = OFPERR_NXBRC_MUST_BE_ZERO;

I'd just return that directly instead of assigning it to 'error'.

> +    }
> +
...
> +}
> +

...

> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 32d3836..f9dc0ca 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -279,6 +279,45 @@ 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 *idstr, *string;
> +        char *cntr;
> +
> +        ids->ofpact.compat = OFPUTIL_NXAST_DEC_TTL_CNT_IDS;
> +        string = xstrdup(arg);

I don't see a corresponding free call.  But I don't think the
xstrdup() is necessary, because I think that this function is allowed
to modify 'arg'.

> +        cntr = strtok_r(string, ", ", &idstr);
> +
> +        for (;;) {
> +            uint16_t id;
> +
> +            id = atoi(cntr);

I don't think it's safe to use 'cntr' without testing it here.  I
believe that it could be NULL even on the first iteration.

The canonical form of this loop, in OVS style anyhow, is:

    for (cntr = strtok_r(string, ", ", &idstr); cntr != NULL;
         cntr = strtok_r(NULL, ", ", &idstr)) {

> +            ofpbuf_put(b, &id, sizeof id);
> +            ids = b->l2;
> +            ids->n_controllers++;
> +
> +            if (*idstr == '\0') {
> +                break;
> +            }
> +            cntr = strtok_r(NULL, ", ", &idstr);
> +        }
> +    }
> +    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)
>  {

...

>  # actions=dec_ttl
> -ffff 0010 00002320 0012 000000000000
> +ffff 0018 00002320 0012 000100000000 0000000000000000

The above is a great illustration of how this commit changes the form
of the existing NXAST_DEC_TTL, even though it must not do that.

Thanks,

Ben.