On Wed, Aug 15, 2012 at 11:51:12AM -0700, Mehak Mahajan wrote: > 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.
Sorry. There are lots of layers these days. Maybe I was not clear enough. I don't mean to cause more work than necessary. > > > + 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. I think that syntax errors such as dec_ttl(,) would also yield NULL. Did you try that? > I will rework the patch and send it out according to your latest comments. Thanks! _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev