[ovs-dev] [PATCH v2 01/15] netdev-vport: Factor-out tunnel Push-pop code into separate module.

[pravin shelar]

On Fri, May 6, 2016 at 11:51 AM, Jesse Gross <jesse at kernel.org> wrote:
> On Thu, Apr 21, 2016 at 6:54 PM, Pravin B Shelar <pshelar at ovn.org> wrote:
>> diff --git a/lib/netdev-native-tnl.h b/lib/netdev-native-tnl.h
>> new file mode 100644
>> index 0000000..5173b10
>> --- /dev/null
>> +++ b/lib/netdev-native-tnl.h
>> +static inline bool
>> +is_header_ipv6(const void *header)
>> +{
>> +    const struct eth_header *eth;
>> +    eth = header;
>> +    return eth->eth_type == htons(ETH_TYPE_IPV6);
>> +}
>> +
>> +static inline struct ip_header *
>> +ip_hdr(void *eth)
>> +{
>> +    return (void *)((char *)eth + sizeof (struct eth_header));
>> +}
>> +
>> +static inline struct ovs_16aligned_ip6_hdr *
>> +ipv6_hdr(void *eth)
>> +{
>> +    return (void *)((char *)eth + sizeof (struct eth_header));
>> +}
>
> These seem fairly generic, maybe packets.h would be a better spot?
>
I think it is not very generic, since it does not parse the eth
protocol to get l3 header. so I will add netted_tnl_ prefix for this
function.
>> +static inline void
>> +pkt_metadata_init_tnl(struct pkt_metadata *md)
>> +{
>> +    /* Zero up through the tunnel metadata options. The length and table
>> +     * are before this and as long as they are empty, the options won't
>> +     * be looked at. */
>> +    memset(md, 0, offsetof(struct pkt_metadata, tunnel.metadata.opts));
>> +}
>> +
>
> And maybe this would make more sense in dp-packet.h?
>
ok.

>> +static inline ovs_be16
>> +get_src_port(struct dp_packet *packet)
>> +{
>> +    uint32_t hash;
>> +
>> +    hash = dp_packet_get_rss_hash(packet);
>> +
>> +    return htons((((uint64_t) hash * (tnl_udp_port_max - tnl_udp_port_min)) >> 32) +
>> +                 tnl_udp_port_min);
>> +}
>> +
>> +void *
>> +ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl,
>> +                  unsigned int *hlen);
>> +void *
>> +push_ip_header(struct dp_packet *packet,
>> +               const void *header, int size, int *ip_tot_size);
>
> It might be a good idea to give these functions a netdev_tnl_ prefix
> in order to make it clear where they came from.
>
ok.

>> +void
>> +netdev_tnl_egress_port_range(struct unixctl_conn *conn, int argc,
>> +                             const char *argv[], void *aux OVS_UNUSED);
>
> Duplicate declaration?
>
removed.

>> diff --git a/lib/netdev-vport-private.h b/lib/netdev-vport-private.h
>> new file mode 100644
>> index 0000000..a087031
>> --- /dev/null
>> +++ b/lib/netdev-vport-private.h
> [...]
>> +struct vport_class {
>> +    const char *dpif_port;
>> +    struct netdev_class netdev_class;
>> +};
>
> Is this definition used anywhere in this file?

right.