| Message ID | 1420577481-20238-1-git-send-email-daniele.di.proietto@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 *);
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(-)