On Wed, May 03, 2017 at 06:07:53PM +0300, Roi Dayan wrote:
> From: Paul Blakey <pa...@mellanox.com>

Please add some text to the changelog.

> 
> Signed-off-by: Paul Blakey <pa...@mellanox.com>
> Reviewed-by: Roi Dayan <r...@mellanox.com>
> Reviewed-by: Simon Horman <simon.hor...@netronome.com>

...

> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> new file mode 100644
> index 0000000..eb5e79a
> --- /dev/null
> +++ b/lib/netdev-tc-offloads.c
> @@ -0,0 +1,154 @@
> +/*
> + * Copyright (c) 2016 Mellanox Technologies, Ltd.
> + *
> + * 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 "netdev-tc-offloads.h"
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <arpa/inet.h>
> +#include <inttypes.h>
> +#include <linux/filter.h>
> +#include <linux/gen_stats.h>
> +#include <linux/if_ether.h>
> +#include <linux/if_tun.h>
> +#include <linux/types.h>
> +#include <linux/ethtool.h>
> +#include <linux/mii.h>
> +#include <linux/pkt_cls.h>
> +#include <linux/pkt_sched.h>
> +#include <linux/rtnetlink.h>
> +#include <linux/sockios.h>
> +#include <sys/types.h>
> +#include <sys/ioctl.h>
> +#include <sys/socket.h>
> +#include <sys/utsname.h>
> +#include <netpacket/packet.h>
> +#include <net/if.h>
> +#include <net/if_arp.h>
> +#include <net/if_packet.h>
> +#include <net/route.h>
> +#include <netinet/in.h>
> +#include <poll.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include "coverage.h"
> +#include "dp-packet.h"
> +#include "dpif-netlink.h"
> +#include "dpif-netdev.h"
> +#include "openvswitch/dynamic-string.h"
> +#include "fatal-signal.h"
> +#include "hash.h"
> +#include "openvswitch/hmap.h"
> +#include "netdev-provider.h"
> +#include "netdev-vport.h"
> +#include "netlink-notifier.h"
> +#include "netlink-socket.h"
> +#include "netlink.h"
> +#include "openvswitch/ofpbuf.h"
> +#include "openflow/openflow.h"
> +#include "ovs-atomic.h"
> +#include "packets.h"
> +#include "poll-loop.h"
> +#include "rtnetlink.h"
> +#include "openvswitch/shash.h"
> +#include "netdev-provider.h"
> +#include "openvswitch/match.h"
> +#include "openvswitch/vlog.h"
> +#include "tc.h"

Are all of the above headers needed for what follows?
There seems to be a lot of #includes above.

> +VLOG_DEFINE_THIS_MODULE(netdev_tc_offloads);
> +
> +int
> +netdev_tc_flow_flush(struct netdev *netdev OVS_UNUSED)
> +{
> +    return EOPNOTSUPP;
> +}
> +
> +int
> +netdev_tc_flow_dump_create(struct netdev *netdev,
> +                           struct netdev_flow_dump **dump_out)
> +{
> +    struct netdev_flow_dump *dump = xzalloc(sizeof *dump);
> +
> +    dump->netdev = netdev_ref(netdev);
> +
> +    *dump_out = dump;
> +
> +    return 0;
> +}
> +
> +int
> +netdev_tc_flow_dump_destroy(struct netdev_flow_dump *dump)
> +{
> +    netdev_close(dump->netdev);
> +    free(dump);
> +
> +    return 0;
> +}
> +
> +bool
> +netdev_tc_flow_dump_next(struct netdev_flow_dump *dump OVS_UNUSED,
> +                         struct match *match OVS_UNUSED,
> +                         struct nlattr **actions OVS_UNUSED,
> +                         struct dpif_flow_stats *stats OVS_UNUSED,
> +                         ovs_u128 *ufid OVS_UNUSED,
> +                         struct ofpbuf *rbuffer OVS_UNUSED,
> +                         struct ofpbuf *wbuffer OVS_UNUSED)
> +{
> +    return false;
> +}
> +
> +int
> +netdev_tc_flow_put(struct netdev *netdev OVS_UNUSED,
> +                   struct match *match OVS_UNUSED,
> +                   struct nlattr *actions OVS_UNUSED,
> +                   size_t actions_len OVS_UNUSED,
> +                   struct dpif_flow_stats *stats OVS_UNUSED,
> +                   const ovs_u128 *ufid OVS_UNUSED,

Here and above ufid follows stats.

> +                   struct offload_info *info OVS_UNUSED)
> +{
> +    return EOPNOTSUPP;
> +}
> +
> +int
> +netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
> +                   struct match *match OVS_UNUSED,
> +                   struct nlattr **actions OVS_UNUSED,
> +                   const ovs_u128 *ufid OVS_UNUSED,
> +                   struct dpif_flow_stats *stats OVS_UNUSED,

But here the order is reversed.

I always think that when a reviewer asks for entries to be sorted they
have run out of things to say. But nonetheless it would be slightly
nicer if the order was consistent unless there is a reason for it not to
be.

> +                   struct ofpbuf *buf OVS_UNUSED)
> +{
> +    return EOPNOTSUPP;
> +}
> +
> +int
> +netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> +                   const ovs_u128 *ufid OVS_UNUSED,
> +                   struct dpif_flow_stats *stats OVS_UNUSED)
> +{
> +    return EOPNOTSUPP;
> +}
> +
> +int
> +netdev_tc_init_flow_api(struct netdev *netdev OVS_UNUSED)
> +{
> +    return 0;
> +}
> +

There is a trailing blank line above.

> diff --git a/lib/netdev-tc-offloads.h b/lib/netdev-tc-offloads.h
> new file mode 100644
> index 0000000..76b7938
> --- /dev/null
> +++ b/lib/netdev-tc-offloads.h
> @@ -0,0 +1,42 @@

> +int netdev_tc_flow_put(struct netdev *, struct match *,
> +                       struct nlattr *actions, size_t actions_len,
> +                       const ovs_u128 *, struct offload_info *,
> +                       struct dpif_flow_stats *);

The declaration of netdev_tc_flow_put() does not match its definition.

...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to