Ben Pfaff <b...@ovn.org> wrote on 07/28/2016 01:02:47 AM:
> From: Ben Pfaff <b...@ovn.org>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: dev@openvswitch.org
> Date: 07/28/2016 01:03 AM
> Subject: Re: [ovs-dev] [PATCH] ovn-controller: Persist desired
> conntrack groups.
>
> On Wed, Jul 27, 2016 at 04:23:07AM +0000, Ryan Moats wrote:
> > [1] indicates that with incremental processing of logical flows
> > desired conntrack groups are not being persisted. This patch
> > adds this capability, with the side effect of adding a ds_copy
> > method that this capability leverages.
> >
> > [1] http://openvswitch.org/pipermail/dev/2016-July/076320.html
>
> Thanks for the fix! Would you mind transforming the above to use
> Reported-at, Reported-by, and Fixes?
Sure, I can do that...
> > Signed-off-by: Ryan Moats <rmo...@us.ibm.com>
>
> > +void
> > +ds_copy(struct ds *dst, struct ds *source)
> > +{
> > + dst->length = source->length;
> > + dst->allocated = source->allocated;
> > + dst->string = xmemdup(source->string, source->allocated + 1);
> > +}
>
> In the case where allocate is much bigger than length, the above is
> pretty wasteful. Would you mind doing separate xmalloc() and memcpy()
> steps?
I went back and forth on this one between keeping it *as is* and doing
xmalloc + memcpy, so I ended up mentally flipping a coin
(and of course, got it wrong), so sure, I can do that.
> We tend to use the term "clone" for operations that initialize and copy
> in one step; "copy" tend to imply copying into an already-initialized
> destination. So I would be more likely to call this ds_clone().
I also went back and forth on "copy" vs "clone" in the name, with the
same mental coin flip/result as above (in case anybody is wondering,
I don't play games of chance for obvious reasons).
> The actions_parse() function has a pretty clear explanatory comment, but
> it doesn't hint at what the new uuid parameter means...
I can certainly fix this.
> Also, should the
> new parameter be part of action_params? It's where most of the
> parameters related to parsing go.
Since I see that have to do a rebase as well, I'm willing to see about
putting this in the action_params and see how it goes...
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
