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
