On Thu, May 23, 2013 at 10:58:46AM -0700, Ben Pfaff wrote:
> Let's use a better name for the new files, say odp-execute.[ch], and
> use an appropriately prefixed name for the function,
> e.g. odp_actions_execute().
Thanks, I have renamed the files as odp-execute.[ch],
and prefixed the functions with odp_
> It would also be acceptable to break
> action-related code out of odp-util.[ch] and put all of it in a new
> odp-actions.[ch].
I'm not sure that I see an advantage in shuffling things around
in that manner at this point.
> Some more comments:
>
> On Wed, May 22, 2013 at 04:08:07PM +0900, Simon Horman wrote:
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index a4ac23f..1717602 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -37,6 +37,7 @@
> > #include "dummy.h"
> > #include "dynamic-string.h"
> > #include "flow.h"
> > +#include "execute-actions.h"
> > #include "hmap.h"
>
> Please keep the #includes in alphabetical order.
Thanks, fixed.
>
> > const struct dpif_class dpif_netdev_class = {
> > diff --git a/lib/execute-actions.c b/lib/execute-actions.c
> > new file mode 100644
> > index 0000000..d1bbeee
> > --- /dev/null
> > +++ b/lib/execute-actions.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> > + * Copyright (c) 2013 Simon Horman
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +#include <linux/openvswitch.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +
> > +#include "execute-actions.h"
>
> #include "execute-actions.h" should go right after #include
> <config.h>, please see CodingStyle.
Thanks, fixed.
>
> > + case OVS_KEY_ATTR_ETHERNET:
> > + eth_set_src_and_dst(packet,
> > + nl_attr_get_unspec(a, sizeof(struct ovs_key_ethernet)));
>
> Please fix indentation here.
Thanks, fixed.
>
> > +#ifndef EXECUTE_ACTIONS_H
> > +#define EXECUTE_ACTIONS_H 1
> > +
> > +#include "flow.h"
> > +#include "netlink-protocol.h"
> > +#include "ofpbuf.h"
>
> It would be better to minimize the header dependencies, more like
> this:
>
> #include <stddef.h>
> #include <stdint.h>
>
> struct flow;
> struct nlattr;
> struct ofpbuf;
Sure, I have updated the patch accordingly.
>
> > +void
> > +execute_actions(void *dp, struct ofpbuf *packet, struct flow *key,
> > + const struct nlattr *actions, size_t actions_len,
> > + void (*output)(void *dp, struct ofpbuf *packet,
> > + uint32_t out_port),
> > + void (*userspace)(void *dp, struct ofpbuf *packet,
> > + const struct flow *key,
> > + const struct nlattr *a));
> > +#endif
>
> Thanks,
>
> Ben.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
