Yes on both counts.
On Wed, Aug 15, 2012 at 11:33:16AM -0700, Mehak Mahajan wrote:
> Hey Ben,
>
> Thanks for your comments.
>
> Just to be sure that I have understood this correctly.
>
> 1. The NXAST_DEC_TTL must continue to use the same nx_action_header but the
> new message NXAST_DEC_TTL_CNT_IDS will use the new data structure ?
> 2. For both we will still use dec_ttl for the syntax .. i.e for the former
> it will be dec_ttl and for the latter it will be dec_ttl(id1,id2) ?
>
> thanx!
> mehak
>
> On Wed, Aug 15, 2012 at 11:23 AM, Ben Pfaff <b...@nicira.com> wrote:
>
> > 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.
> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
