[ovs-dev] [PATCH v3 2/5] ovs-ofctl: Handle any number of buckets in group statistics

[Ben Pfaff]

On Fri, Sep 06, 2013 at 10:57:26AM +0900, Simon Horman wrote:
> On Thu, Sep 05, 2013 at 12:58:40PM -0700, Ben Pfaff wrote:
> > On Thu, Sep 05, 2013 at 12:55:28PM -0700, Ben Pfaff wrote:
> > > On Thu, Sep 05, 2013 at 02:19:13PM +0900, Simon Horman wrote:
> > > > struct ofputil_group_stats has an arbitrary limit
> > > > of 16 buckets for which it can record statistics.
> > > > However the code does not appear to enforce this
> > > > limit and it seems to me that the code could overflow.
> > > > 
> > > > This patch aims to remove the arbitrary limit by
> > > > changing the 'bucket_stats' field of struct ofputil_group_stats
> > > > from a fixed length array to a pointer whose storage is allocated and freed
> > > > as necessary.
> > > > 
> > > > Signed-off-by: Simon Horman <horms at verge.net.au>
> > > > 
> > > > ---
> > > > v3
> > > > * As suggested by Ben Pfaff
> > > >   - Vastly simplify the change by using an explicit pointer for
> > > >     the 'bucket_stats' member of struct ofputil_group_stats rather
> > > >     than implicit variable length array appended to the end of the
> > > >     structure.
> > > 
> > > Thanks!
> > > 
> > > It looked to me that ofp_print_group_stats() shouldn't free
> > > gs.bucket_stats on error because in that case gs.bucket_stats won't be
> > > properly initialized, so I changed that.  And then I changed
> > > ofputil_decode_group_stats_reply() to always initialize
> > > gs.bucket_stats as a second measure of protection.
> > > 
> > > I folded in the following and will apply patches 1 and 2 soon.
> > > 
> > > I'll review patches 3 through 5 tomorrow during the hackathon.
> > > (Thanks so much for participating!)
> > > 
> > > --8<--------------------------cut here-------------------------->8--
> > > 
> > > diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> > > index a794e70..6433dd0 100644
> > > --- a/lib/ofp-print.c
> > > +++ b/lib/ofp-print.c
> > > @@ -2229,7 +2229,6 @@ ofp_print_group_stats(struct ds *s, const struct ofp_header *oh)
> > >              if (retval != EOF) {
> > >                  ds_put_cstr(s, " ***parse error***");
> > >              }
> > > -            free(gs.bucket_stats);
> > >              break;
> > >          }
> > >  
> > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > > index 6573025..8ac9186 100644
> > > --- a/lib/ofp-util.c
> > > +++ b/lib/ofp-util.c
> > > @@ -5480,7 +5480,8 @@ ofputil_decode_group_stats_request(const struct ofp_header *request,
> > >  }
> > >  
> > >  /* Converts a group stats reply in 'msg' into an abstract ofputil_group_stats
> > > - * in 'gs'.
> > > + * in 'gs'.  Assigns freshly allocated memory to gs->bucket_stats for the
> > > + * caller to eventually free.
> > >   *
> > >   * Multiple group stats replies can be packed into a single OpenFlow message.
> > >   * Calling this function multiple times for a single 'msg' iterates through the
> > > @@ -5501,6 +5502,7 @@ ofputil_decode_group_stats_reply(struct ofpbuf *msg,
> > >      size_t length;
> > >      size_t i;
> > >  
> > > +    gs->bucket_stats = NULL;
> > >      error = (msg->l2
> > >               ? ofpraw_decode(&raw, msg->l2)
> > >               : ofpraw_pull(&raw, msg));
> > 
> > Oh, I see now that you had 'gs.bucket_stats = NULL;' in
> > ofp_print_group_stats() just before calling
> > ofputil_decode_group_stats_reply(), so this wasn't actually a bug.
> > Still, I think it's better to put it in
> > ofputil_decode_group_stats_reply() itself.
> 
> Thanks, that seems like a good approach to me.
> Perhaps we should remove 'gs.bucket_stats = NULL;' from
> ofp_print_group_stats() as it seems to be unnecessary now.

I did that before I pushed the commit, but I forgot to mention it here.