[ovs-dev] [PATCH] ofp-util: Reject bad group type and command with error instead of abort.

[Ben Pfaff]

On Mon, Nov 30, 2015 at 09:05:54PM -0200, Flavio Leitner wrote:
> On Sun, Nov 29, 2015 at 11:02:02AM -0800, Ben Pfaff wrote:
> > On Thu, Nov 26, 2015 at 05:20:54PM -0200, Flavio Leitner wrote:
> > > On Mon, Oct 12, 2015 at 10:10:27AM -0700, Ben Pfaff wrote:
> > > > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > > > Reported-by: Manpreet Singh <er.manpreet25 at gmail.com>
> > > > Reported-at: http://openvswitch.org/pipermail/discuss/2015-October/019048.html
> > > > ---
> > > >  AUTHORS        | 1 +
> > > >  lib/ofp-util.c | 4 ++--
> > > >  2 files changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/AUTHORS b/AUTHORS
> > > > index 99bcf60..8123f43 100644
> > > > --- a/AUTHORS
> > > > +++ b/AUTHORS
> > > > @@ -310,6 +310,7 @@ Len Gao                 leng at vmware.com
> > > >  Logan Rosen             logatronico at gmail.com
> > > >  Luca Falavigna          dktrkranz at debian.org
> > > >  Luiz Henrique Ozaki     luiz.ozaki at gmail.com
> > > > +Manpreet Singh          er.manpreet25 at gmail.com
> > > >  Marco d'Itri            md at Linux.IT
> > > >  Martin Vizvary          vizvary at ics.muni.cz
> > > >  Marvin Pascual          marvin at pascual.com.ph
> > > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > > > index b9dbcda..f0f6319 100644
> > > > --- a/lib/ofp-util.c
> > > > +++ b/lib/ofp-util.c
> > > > @@ -8679,7 +8679,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh,
> > > >      case OFPGT11_FF:
> > > >          break;
> > > >      default:
> > > > -        OVS_NOT_REACHED();
> > > > +        return OFPERR_OFPGMFC_BAD_TYPE;
> > > 
> > > This looks correct.
> > > 
> > > 
> > > >      }
> > > >  
> > > >      switch (gm->command) {
> > > > @@ -8694,7 +8694,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh,
> > > >          }
> > > >          break;
> > > >      default:
> > > > -        OVS_NOT_REACHED();
> > > > +        return OFPERR_OFPGMFC_BAD_COMMAND;
> > > 
> > > This too.
> > > >      }
> > > >  
> > > >      LIST_FOR_EACH (bucket, list_node, &gm->buckets) {
> > > 
> > > But then it continues iterating over the buckets checking the
> > > gm->type:
> > > ...
> > >         default:
> > >             OVS_NOT_REACHED();
> > >         }
> > > Shouldn't that also returns OFPERR_OFPGMFC_BAD_TYPE?
> > 
> > Here we genuinely can't get any invalid types because the first "switch"
> > statement in the function has verified gm->type.
> 
> Indeed, that is the case now.  However, if we copy&paste that piece of
> code somewhere else or change the function in the future, then the issue
> might get reintroduced.

OK, I sent out a patch, CCing you.