Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support
On 19/11/2024 03:23, Sergey Ryazanov wrote:
On 15.11.2024 12:19, Antonio Quartulli wrote:
On 09/11/2024 00:31, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
+/**
+ * struct ovpn_struct - per ovpn interface state
+ * @dev: the actual netdev representing the tunnel
+ * @dev_tracker: reference tracker for associated dev
+ */
+struct ovpn_struct {
There is no standard convention how to entitle such structures, so
the question is basically of out-of-curiosity class. For me, having a
sturcuture with name 'struct' is like having no name. Did you
consider to use such names as ovpn_dev or ovpn_iface? Meaning, using
a name that gives a clue regarding the scope of the content.
Yes, I wanted to switch to ovpn_priv, but did not care much for the
time being :)
I can still do it now in v12.
This topic caused me the biggest doubts. I don't want to ask to rename
everything on the final lap. Just want to share an outside perspective
on the structure name. And let you decide is it worth or not.
And if you ask me, ovpn_priv does not give a clue either. The module is
too complex for a vague structure name, even after your great work on
clearing its design.
Well, the word "priv" to me resembles the "netdev_priv()" call, so it's
kinda easier to grasp what this is about.
In batman-adv we used the same suffix and it was well received.
Also, if you grep for "_priv " in drivers/net you will see that this is
a common pattern.
Since I already had in mind to change this struct name, I moved on and
renamed it to ovpn_priv throughput the patchset (git rebase --exec is my
friend ;)).
Thanks
Regards,
--
Sergey
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support
On 19/11/2024 03:05, Sergey Ryazanov wrote:
On 15.11.2024 12:05, Antonio Quartulli wrote:
On 09/11/2024 00:15, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
@@ -37,7 +41,7 @@ static int ovpn_newlink(struct net *src_net,
struct net_device *dev,
}
static struct rtnl_link_ops ovpn_link_ops = {
- .kind = "ovpn",
+ .kind = OVPN_FAMILY_NAME,
nit: are you sure that the link kind is the same as the GENL family?
I mean, they are both deriviated from the protocol name that is
common for both entities, but is it making RTNL kind a derivative of
GENL family?
I just want to use the same name everywhere and I thought it doesn't
make sense to create a separate define (they can be decoupled later
should see any need for that).
But I can add:
#define OVPN_RTNL_LINK_KIND OVPN_FAMILY_NAME
to make this relationship explicit?
Can we just leave it as literal? This string is going to be a part of
ABI and there will be no chance to change it in the future. So, what the
purpose to define it using a macro if it's self-descriptive?
I don't truly have a strong opinion, but the netlink family name is also
expected to not change anytime soon.
Anyway, I see that using the literal is pretty common across all other
drivers, therefore I'll go for it as well.
People also like to define a macro with a generic name like DRV_NAME and
use it everywhere. What also looks reasonable.
Yeah, that's exactly how I am using OVPN_FAMILY_NAME.
Anyway, I am switching to literal.
Regards,
--
Sergey
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support
On 15.11.2024 12:19, Antonio Quartulli wrote:
On 09/11/2024 00:31, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
+/**
+ * struct ovpn_struct - per ovpn interface state
+ * @dev: the actual netdev representing the tunnel
+ * @dev_tracker: reference tracker for associated dev
+ */
+struct ovpn_struct {
There is no standard convention how to entitle such structures, so the
question is basically of out-of-curiosity class. For me, having a
sturcuture with name 'struct' is like having no name. Did you consider
to use such names as ovpn_dev or ovpn_iface? Meaning, using a name
that gives a clue regarding the scope of the content.
Yes, I wanted to switch to ovpn_priv, but did not care much for the
time being :)
I can still do it now in v12.
This topic caused me the biggest doubts. I don't want to ask to rename
everything on the final lap. Just want to share an outside perspective
on the structure name. And let you decide is it worth or not.
And if you ask me, ovpn_priv does not give a clue either. The module is
too complex for a vague structure name, even after your great work on
clearing its design.
--
Sergey
Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support
On 15.11.2024 12:05, Antonio Quartulli wrote:
On 09/11/2024 00:15, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
@@ -37,7 +41,7 @@ static int ovpn_newlink(struct net *src_net, struct
net_device *dev,
}
static struct rtnl_link_ops ovpn_link_ops = {
- .kind = "ovpn",
+ .kind = OVPN_FAMILY_NAME,
nit: are you sure that the link kind is the same as the GENL family? I
mean, they are both deriviated from the protocol name that is common
for both entities, but is it making RTNL kind a derivative of GENL
family?
I just want to use the same name everywhere and I thought it doesn't
make sense to create a separate define (they can be decoupled later
should see any need for that).
But I can add:
#define OVPN_RTNL_LINK_KIND OVPN_FAMILY_NAME
to make this relationship explicit?
Can we just leave it as literal? This string is going to be a part of
ABI and there will be no chance to change it in the future. So, what the
purpose to define it using a macro if it's self-descriptive?
People also like to define a macro with a generic name like DRV_NAME and
use it everywhere. What also looks reasonable.
--
Sergey
Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support
On 09/11/2024 00:31, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
This commit introduces basic netlink support with family
registration/unregistration functionalities and stub pre/post-doit.
More importantly it introduces the YAML uAPI description along
with its auto-generated files:
- include/uapi/linux/ovpn.h
- drivers/net/ovpn/netlink-gen.c
- drivers/net/ovpn/netlink-gen.h
Cc: donald.hun...@gmail.com
Signed-off-by: Antonio Quartulli
[skipped]
diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/
ovpnstruct.h
--- /dev/null
+++ b/drivers/net/ovpn/ovpnstruct.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* OpenVPN data channel offload
+ *
+ * Copyright (C) 2019-2024 OpenVPN, Inc.
+ *
+ * Author: James Yonan
+ * Antonio Quartulli
+ */
+
+#ifndef _NET_OVPN_OVPNSTRUCT_H_
+#define _NET_OVPN_OVPNSTRUCT_H_
+
+#include
+
+/**
+ * struct ovpn_struct - per ovpn interface state
+ * @dev: the actual netdev representing the tunnel
+ * @dev_tracker: reference tracker for associated dev
+ */
+struct ovpn_struct {
There is no standard convention how to entitle such structures, so the
question is basically of out-of-curiosity class. For me, having a
sturcuture with name 'struct' is like having no name. Did you consider
to use such names as ovpn_dev or ovpn_iface? Meaning, using a name that
gives a clue regarding the scope of the content.
Yes, I wanted to switch to ovpn_priv, but did not care much for the
time being :)
I can still do it now in v12.
Thanks!
Regards,
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support
On 09/11/2024 00:15, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
This commit introduces basic netlink support with family
registration/unregistration functionalities and stub pre/post-doit.
More importantly it introduces the YAML uAPI description along
with its auto-generated files:
- include/uapi/linux/ovpn.h
- drivers/net/ovpn/netlink-gen.c
- drivers/net/ovpn/netlink-gen.h
Cc: donald.hun...@gmail.com
Signed-off-by: Antonio Quartulli
[skipped]
diff --git a/Documentation/netlink/specs/ovpn.yaml b/Documentation/
netlink/specs/ovpn.yaml
[skipped]
+attribute-sets:
+ -
+ name: peer
+ attributes:
+ -
+ name: id
+ type: u32
+ doc: |
+ The unique ID of the peer. To be used to identify peers during
+ operations
nit: could you specify the scope of uniqueness? I believe it is not
globally uniq, it is just interface uniq, right?
Yeah it's per interface/instance.
Will make it more clear, also for other IDs.
+ checks:
+ max: 0xFF
[skipped]
diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
index
369a5a2b2fc1a497c8444e59f9b058eb40e49524..d5bdb0055f4dd3a6e32dc6e792bed1e7fd59e101 100644
--- a/drivers/net/ovpn/main.c
+++ b/drivers/net/ovpn/main.c
@@ -7,11 +7,15 @@
* James Yonan
*/
+#include
#include
#include
#include
+#include
+#include "ovpnstruct.h"
#include "main.h"
+#include "netlink.h"
#include "io.h"
/* Driver info */
@@ -37,7 +41,7 @@ static int ovpn_newlink(struct net *src_net, struct
net_device *dev,
}
static struct rtnl_link_ops ovpn_link_ops = {
- .kind = "ovpn",
+ .kind = OVPN_FAMILY_NAME,
nit: are you sure that the link kind is the same as the GENL family? I
mean, they are both deriviated from the protocol name that is common for
both entities, but is it making RTNL kind a derivative of GENL family?
I just want to use the same name everywhere and I thought it doesn't
make sense to create a separate define (they can be decoupled later
should see any need for that).
But I can add:
#define OVPN_RTNL_LINK_KIND OVPN_FAMILY_NAME
to make this relationship explicit?
Regards,
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support
On 29.10.2024 12:47, Antonio Quartulli wrote:
This commit introduces basic netlink support with family
registration/unregistration functionalities and stub pre/post-doit.
More importantly it introduces the YAML uAPI description along
with its auto-generated files:
- include/uapi/linux/ovpn.h
- drivers/net/ovpn/netlink-gen.c
- drivers/net/ovpn/netlink-gen.h
Cc: donald.hun...@gmail.com
Signed-off-by: Antonio Quartulli
[skipped]
diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/ovpnstruct.h
--- /dev/null
+++ b/drivers/net/ovpn/ovpnstruct.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* OpenVPN data channel offload
+ *
+ * Copyright (C) 2019-2024 OpenVPN, Inc.
+ *
+ * Author:James Yonan
+ * Antonio Quartulli
+ */
+
+#ifndef _NET_OVPN_OVPNSTRUCT_H_
+#define _NET_OVPN_OVPNSTRUCT_H_
+
+#include
+
+/**
+ * struct ovpn_struct - per ovpn interface state
+ * @dev: the actual netdev representing the tunnel
+ * @dev_tracker: reference tracker for associated dev
+ */
+struct ovpn_struct {
There is no standard convention how to entitle such structures, so the
question is basically of out-of-curiosity class. For me, having a
sturcuture with name 'struct' is like having no name. Did you consider
to use such names as ovpn_dev or ovpn_iface? Meaning, using a name that
gives a clue regarding the scope of the content.
For interface functions, when the pointer assigned in such manner as
`ovpn = netdev_priv(dev)`, it is clear what is inside. But for functions
like ovpn_peer_get_by_id() it is a bit tricky to quickly realize, what
is this for.
+ struct net_device *dev;
+ netdevice_tracker dev_tracker;
+};
+
+#endif /* _NET_OVPN_OVPNSTRUCT_H_ */
--
Sergey
Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support
On 29.10.2024 12:47, Antonio Quartulli wrote:
This commit introduces basic netlink support with family
registration/unregistration functionalities and stub pre/post-doit.
More importantly it introduces the YAML uAPI description along
with its auto-generated files:
- include/uapi/linux/ovpn.h
- drivers/net/ovpn/netlink-gen.c
- drivers/net/ovpn/netlink-gen.h
Cc: donald.hun...@gmail.com
Signed-off-by: Antonio Quartulli
[skipped]
diff --git a/Documentation/netlink/specs/ovpn.yaml
b/Documentation/netlink/specs/ovpn.yaml
[skipped]
+attribute-sets:
+ -
+name: peer
+attributes:
+ -
+name: id
+type: u32
+doc: |
+ The unique ID of the peer. To be used to identify peers during
+ operations
nit: could you specify the scope of uniqueness? I believe it is not
globally uniq, it is just interface uniq, right?
+checks:
+ max: 0xFF
[skipped]
diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
index
369a5a2b2fc1a497c8444e59f9b058eb40e49524..d5bdb0055f4dd3a6e32dc6e792bed1e7fd59e101
100644
--- a/drivers/net/ovpn/main.c
+++ b/drivers/net/ovpn/main.c
@@ -7,11 +7,15 @@
*James Yonan
*/
+#include
#include
#include
#include
+#include
+#include "ovpnstruct.h"
#include "main.h"
+#include "netlink.h"
#include "io.h"
/* Driver info */
@@ -37,7 +41,7 @@ static int ovpn_newlink(struct net *src_net, struct
net_device *dev,
}
static struct rtnl_link_ops ovpn_link_ops = {
- .kind = "ovpn",
+ .kind = OVPN_FAMILY_NAME,
nit: are you sure that the link kind is the same as the GENL family? I
mean, they are both deriviated from the protocol name that is common for
both entities, but is it making RTNL kind a derivative of GENL family?
.netns_refund = false,
.newlink = ovpn_newlink,
.dellink = unregister_netdevice_queue,
@@ -93,8 +97,16 @@ static int __init ovpn_init(void)
goto unreg_netdev;
}
--
Sergey
