[PATCH net-next] openvswitch: Do not use private netdev_vport fields

[PATCH net-next] openvswitch: Do not use private netdev_vport fields

[Daniele Di Proietto]

This commit introduces netdev_vport_index() to prevent datapath.c from directly accessing the 'dev' member of 'struct netdev_vport'.
This fix is needed to allow possible alternative netdev_vport implementations.

Signed-off-by: Daniele Di Proietto <daniele.di.proietto@gmail.com>
---
 net/openvswitch/datapath.c     | 2 +-
 net/openvswitch/vport-netdev.h | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 4e9a5f0..d632535 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -186,7 +186,7 @@ static int get_dpifindex(const struct datapath *dp)
 
 	local = ovs_vport_rcu(dp, OVSP_LOCAL);
 	if (local)
-		ifindex = netdev_vport_priv(local)->dev->ifindex;
+		ifindex = netdev_vport_index(local);
 	else
 		ifindex = 0;
 
diff --git a/net/openvswitch/vport-netdev.h b/net/openvswitch/vport-netdev.h
index 6f7038e..ecfcbd5 100644
--- a/net/openvswitch/vport-netdev.h
+++ b/net/openvswitch/vport-netdev.h
@@ -38,6 +38,12 @@ netdev_vport_priv(const struct vport *vport)
 	return vport_priv(vport);
 }
 
+static inline int
+netdev_vport_index(const struct vport *vport)
+{
+	return netdev_vport_priv(vport)->dev->ifindex;
+}
+
 const char *ovs_netdev_get_name(const struct vport *);
 void ovs_netdev_detach_dev(struct vport *);
 
-- 
2.1.4

Re: [PATCH net-next] openvswitch: Do not use private netdev_vport fields

[Pravin Shelar]

On Tue, Jan 6, 2015 at 12:51 PM, Daniele Di Proietto
<daniele.di.proietto@gmail.com> wrote:
> This commit introduces netdev_vport_index() to prevent datapath.c from directly accessing the 'dev' member of 'struct netdev_vport'.
> This fix is needed to allow possible alternative netdev_vport implementations.
>
> Signed-off-by: Daniele Di Proietto <daniele.di.proietto@gmail.com>
> ---
>  net/openvswitch/datapath.c     | 2 +-
>  net/openvswitch/vport-netdev.h | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
...
>
> diff --git a/net/openvswitch/vport-netdev.h b/net/openvswitch/vport-netdev.h
> index 6f7038e..ecfcbd5 100644
> --- a/net/openvswitch/vport-netdev.h
> +++ b/net/openvswitch/vport-netdev.h
> @@ -38,6 +38,12 @@ netdev_vport_priv(const struct vport *vport)
>         return vport_priv(vport);
>  }
>
> +static inline int
> +netdev_vport_index(const struct vport *vport)
> +{
> +       return netdev_vport_priv(vport)->dev->ifindex;
> +}
> +
Function return type and function name should be on same line,
otherwise looks good.

>  const char *ovs_netdev_get_name(const struct vport *);
>  void ovs_netdev_detach_dev(struct vport *);
>
> --
> 2.1.4
>

Re: [PATCH net-next] openvswitch: Do not use private netdev_vport fields

[David Miller]

From: Daniele Di Proietto <daniele.di.proietto@gmail.com>
Date: Tue,  6 Jan 2015 21:51:21 +0100

> This commit introduces netdev_vport_index() to prevent datapath.c from directly accessing the 'dev' member of 'struct netdev_vport'.
> This fix is needed to allow possible alternative netdev_vport implementations.
> 
> Signed-off-by: Daniele Di Proietto <daniele.di.proietto@gmail.com>

This doesn't make any sense to me, as the code currently stands your
change is not necessary at all.

If some need does arise, submit this patch along with the change
that creates the need.

Re: [PATCH net-next] openvswitch: Do not use private netdev_vport fields

[David Miller]

From: Pravin Shelar <pshelar@nicira.com>
Date: Tue, 6 Jan 2015 13:16:11 -0800

> Function return type and function name should be on same line,
> otherwise looks good.

I disagree, where is the code in the tree that needs this?

Re: [PATCH net-next] openvswitch: Do not use private netdev_vport fields

[Pravin Shelar]

On Tue, Jan 6, 2015 at 2:02 PM, David Miller <davem@davemloft.net> wrote:
> From: Pravin Shelar <pshelar@nicira.com>
> Date: Tue, 6 Jan 2015 13:16:11 -0800
>
>> Function return type and function name should be on same line,
>> otherwise looks good.
>
> I disagree, where is the code in the tree that needs this?

Most of function definitions that I have seen are defined like this. I
was pointing out coding style issue.

Re: [PATCH net-next] openvswitch: Do not use private netdev_vport fields

[Pravin Shelar]

On Tue, Jan 6, 2015 at 2:15 PM, Pravin Shelar <pshelar@nicira.com> wrote:
> On Tue, Jan 6, 2015 at 2:02 PM, David Miller <davem@davemloft.net> wrote:
>> From: Pravin Shelar <pshelar@nicira.com>
>> Date: Tue, 6 Jan 2015 13:16:11 -0800
>>
>>> Function return type and function name should be on same line,
>>> otherwise looks good.
>>
>> I disagree, where is the code in the tree that needs this?
>
> Most of function definitions that I have seen are defined like this. I
> was pointing out coding style issue.

About the actual change, I think it is a cleanup. netdev_vport_index()
hides the implementation from datapath.c. I hope Daniele will explain
need for the change.

Re: [PATCH net-next] openvswitch: Do not use private netdev_vport fields

[Daniele Di Proietto]

The motivation for the change is to make datapath.c independent from
the actual implementation of the vport. The problem came up when
experimenting with other vport implementations and this type of change
will help identifying layering violations.
You are perfectly right, however, that in this form the code does not
improve much: we should rather provide a vport_index() call, and
implement one in each of the vports.

Thoughts?

2015-01-06 23:28 GMT+01:00 Pravin Shelar <pshelar@nicira.com>:
> On Tue, Jan 6, 2015 at 2:15 PM, Pravin Shelar <pshelar@nicira.com> wrote:
>> On Tue, Jan 6, 2015 at 2:02 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Pravin Shelar <pshelar@nicira.com>
>>> Date: Tue, 6 Jan 2015 13:16:11 -0800
>>>
>>>> Function return type and function name should be on same line,
>>>> otherwise looks good.
>>>
>>> I disagree, where is the code in the tree that needs this?
>>
>> Most of function definitions that I have seen are defined like this. I
>> was pointing out coding style issue.
>
> About the actual change, I think it is a cleanup. netdev_vport_index()
> hides the implementation from datapath.c. I hope Daniele will explain
> need for the change.

Re: [PATCH net-next] openvswitch: Do not use private netdev_vport fields

[Pravin Shelar]

On Tue, Jan 6, 2015 at 3:32 PM, Daniele Di Proietto
<daniele.di.proietto@gmail.com> wrote:
> The motivation for the change is to make datapath.c independent from
> the actual implementation of the vport. The problem came up when
> experimenting with other vport implementations and this type of change
> will help identifying layering violations.
> You are perfectly right, however, that in this form the code does not
> improve much: we should rather provide a vport_index() call, and
> implement one in each of the vports.
>

This sounds like lot more invasive change compared to the current
patch. For such change I need to see complete set of changes that you
are planning.


> Thoughts?
>
> 2015-01-06 23:28 GMT+01:00 Pravin Shelar <pshelar@nicira.com>:
>> On Tue, Jan 6, 2015 at 2:15 PM, Pravin Shelar <pshelar@nicira.com> wrote:
>>> On Tue, Jan 6, 2015 at 2:02 PM, David Miller <davem@davemloft.net> wrote:
>>>> From: Pravin Shelar <pshelar@nicira.com>
>>>> Date: Tue, 6 Jan 2015 13:16:11 -0800
>>>>
>>>>> Function return type and function name should be on same line,
>>>>> otherwise looks good.
>>>>
>>>> I disagree, where is the code in the tree that needs this?
>>>
>>> Most of function definitions that I have seen are defined like this. I
>>> was pointing out coding style issue.
>>
>> About the actual change, I think it is a cleanup. netdev_vport_index()
>> hides the implementation from datapath.c. I hope Daniele will explain
>> need for the change.

Re: [PATCH net-next] openvswitch: Do not use private netdev_vport fields

[Daniele Di Proietto]

Ok, I've sent the other version of the patch (openvswitch: Add
ovs_vport_get_index() to hide vport implementation).
It adds the .get_index() vport operation (which mimics .get_name())
and ovs_vport_get_index().

Please, let me know which approach you would prefer

Thanks

2015-01-07 7:00 GMT+01:00 Pravin Shelar <pshelar@nicira.com>:
> On Tue, Jan 6, 2015 at 3:32 PM, Daniele Di Proietto
> <daniele.di.proietto@gmail.com> wrote:
>> The motivation for the change is to make datapath.c independent from
>> the actual implementation of the vport. The problem came up when
>> experimenting with other vport implementations and this type of change
>> will help identifying layering violations.
>> You are perfectly right, however, that in this form the code does not
>> improve much: we should rather provide a vport_index() call, and
>> implement one in each of the vports.
>>
>
> This sounds like lot more invasive change compared to the current
> patch. For such change I need to see complete set of changes that you
> are planning.
>
>
>> Thoughts?
>>
>> 2015-01-06 23:28 GMT+01:00 Pravin Shelar <pshelar@nicira.com>:
>>> On Tue, Jan 6, 2015 at 2:15 PM, Pravin Shelar <pshelar@nicira.com> wrote:
>>>> On Tue, Jan 6, 2015 at 2:02 PM, David Miller <davem@davemloft.net> wrote:
>>>>> From: Pravin Shelar <pshelar@nicira.com>
>>>>> Date: Tue, 6 Jan 2015 13:16:11 -0800
>>>>>
>>>>>> Function return type and function name should be on same line,
>>>>>> otherwise looks good.
>>>>>
>>>>> I disagree, where is the code in the tree that needs this?
>>>>
>>>> Most of function definitions that I have seen are defined like this. I
>>>> was pointing out coding style issue.
>>>
>>> About the actual change, I think it is a cleanup. netdev_vport_index()
>>> hides the implementation from datapath.c. I hope Daniele will explain
>>> need for the change.