Hey Ben,
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.
>
>
It seems I misunderstood what you meant the first time. I will change back
NXAST_DEC_TTL to its earlier behavior.
> > + 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.)
> */
>
>
Makes sense.
> > --- 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.
>
>
Sure. Makes sense.
> > +{
> ...
> > +}
> > +
> > +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'.
>
>
Sure. Makes sense.
> > + }
> > +
> ...
> > +}
> > +
>
> ...
>
> > 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'.
>
>
Makes sense.
> > + 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.
>
>
Makes sense.
As a side note, there is a check on the top to see if *arg == '\0' which is
truly the only case in which cntr would be null. In every other case cntr
would return the string in the () ... I also tested the code with just
dec_ttl(), and it returned *arg = '\0' and hence went into the
NXAST_DEC_TTL code snippet.
May you tell me, for my understanding, what is the other cases in which you
feel cntr would be null (so that I can use that as an example when testing
my code) ?
> 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.
>
> Yes. I was aware that the form of dec_ttl was changing. Somehow, I dont
know why, I thought that was expected.
I will rework the patch and send it out according to your latest comments.
> Thanks,
>
> Ben.
>
thanx!
mehak
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
