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

Reply via email to