[ovs-dev] [PATCH] Add Nicira vendor extension action NXAST_DEC_TTL_CNT_IDS.

Mehak Mahajan mmahajan at nicira.com
Wed Aug 15 20:40:45 UTC 2012


On Wed, Aug 15, 2012 at 1:20 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Wed, Aug 15, 2012 at 11:51:12AM -0700, Mehak Mahajan wrote:
> > On Wed, Aug 15, 2012 at 11:23 AM, Ben Pfaff <blp at 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?
>

Yes. I tried out dec_ttl(,) after your comments, and it yields a segfault.
Thanks. I will be sure to try out all these cases before sending out the
next patch.


>
> > I will rework the patch and send it out according to your latest
> comments.
>
> Thanks!
>

Thanks for your help Ben.

Thanks!
Mehak
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20120815/7c6a4eb9/attachment-0003.html>


More information about the dev mailing list