Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support

[Sergey Ryazanov]

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.hunter@xxxxxxxxx
> Signed-off-by: Antonio Quartulli <antonio@xxxxxxxxxxx>

[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 <james@xxxxxxxxxxx>
> + *		Antonio Quartulli <antonio@xxxxxxxxxxx>
> + */
> +
> +#ifndef _NET_OVPN_OVPNSTRUCT_H_
> +#define _NET_OVPN_OVPNSTRUCT_H_
> +
> +#include <net/net_trackers.h>
> +
> +/**
> + * 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