[ovs-dev] [PATCH v3 1/3] Add basic implementation for OpenFlow 1.4 bundles

[Ben Pfaff]

On Fri, Apr 11, 2014 at 03:44:47PM +0300, Alexandru Copot wrote:
> Thanks a lot for the feedback. You can find my answers inline.
> We will get back with v4 asap.

Great!  There is no rush: I'll be out on vacation all next week, so I
will probably not get to it quickly, even if you send the new version
today.

> On Thu, Apr 10, 2014 at 9:38 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Fri, Apr 04, 2014 at 11:58:29AM +0300, Alexandru Copot wrote:
> > I don't think it's necessary for ofputil_decode_bundle_add() to make a
> > malloc()'d clone of the inner message.  Why does it do that?  (If
> > there is a good reason then xmemdup is a better way to do it.)
> 
> I needed a way to access the entire contents of the inner message.
> If I just copy the pointers, the outer message will be freed by the time
> we apply the inner message.

I think it would be better for the caller to make the copy, if
necessary.  (The decoder can point out where the inner message
starts.)  Then code that doesn't need the copy (I think this would
include ofp-print, for example) doesn't get it, and doesn't have the
responsibility of freeing it.

> > At first glance it seems to me that bundles would only be relevant in
> > the ofproto main thread, so I don't know why there is locking.
> 
> I thought they might be needed in the future. We might remove them for now.

I think that we should remove them, for now.  Locking always makes me
look for the reason, and when there isn't a reason it is confusing.

Thanks,

Ben.