[ovs-dev] [PATCH 2/3] tunneling: Remove struct tnl_vport and tnl_ops.

[Jesse Gross]

On Thu, Apr 18, 2013 at 4:51 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/vport-vxlan.c b/datapath/vport-vxlan.c
> index 1850fc2..a5ad3fc 100644
> --- a/datapath/vport-vxlan.c
> +++ b/datapath/vport-vxlan.c
> @@ -62,22 +57,28 @@ static inline int vxlan_hdr_len(const struct ovs_key_ipv4_tunnel *tun_key)
>   * @socket: The socket created for this port number.
>   */
>  struct vxlan_port {
> +       __be16 dst_port;
>         struct list_head list;
>         struct vport *vport;
>         struct socket *vxlan_rcv_socket;
> +       char name[IFNAMSIZ];
>         struct rcu_head rcu;
>  };

We should update the comments to match.

> @@ -230,22 +211,15 @@ static int vxlan_tunnel_setup(struct net *net, struct vport *vport,
>         }
>
>         /* Verify if we already have a socket created for this port */
> -       vxlan_port = vxlan_find_port(net, htons(dst_port));
> -       if (vxlan_port) {
> +       if (vxlan_find_port(net, htons(dst_port))) {
>                 err = -EEXIST;
>                 goto out;
>         }
>
> -       /* Add a new socket for this port */
> -       vxlan_port = kzalloc(sizeof(struct vxlan_port), GFP_KERNEL);
> -       if (!vxlan_port) {
> -               err = -ENOMEM;
> -               goto out;
> -       }
> -
> -       tnl_vport->dst_port = htons(dst_port);
> +       vxlan_port->dst_port = htons(dst_port);
>         vxlan_port->vport = vport;

I think we could now use vport_from_priv() instead of storing an
explicit pointer to the vport.

> @@ -292,28 +258,41 @@ static struct vport *vxlan_tnl_create(const struct vport_parms *parms)
>         int err;
>         struct vport *vport;
>
> -       vport = ovs_tnl_create(parms, &ovs_vxlan_vport_ops, &ovs_vxlan_tnl_ops);
> +       vport = ovs_vport_alloc(sizeof(struct vxlan_port),
> +                               &ovs_vxlan_vport_ops, parms);
>         if (IS_ERR(vport))
>                 return vport;
>
>         err = vxlan_tunnel_setup(ovs_dp_get_net(parms->dp), vport,
> -                                parms->options);
> +                                parms);

I think we can merge together vxlan_tnl_create() and
vxlan_tunnel_setup() since there is now a 1:1 mapping between the two
and they are logically the same operation.

Can factor out the common socket_init from VXLAN and LISP?

Otherwise, similar comments also apply to LISP.

> diff --git a/datapath/vport.h b/datapath/vport.h
> index 8280eba..565ae4f 100644
> --- a/datapath/vport.h
> +++ b/datapath/vport.h
> @@ -51,6 +51,7 @@ int ovs_vport_set_options(struct vport *, struct nlattr *options);
>  int ovs_vport_get_options(const struct vport *, struct sk_buff *);
>
>  int ovs_vport_send(struct vport *, struct sk_buff *);
> +void ovs_vport_deferred_destroy(struct vport *vport);

This should be grouped with ovs_vport_free().