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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <21c0887b-1c7d-424d-a723-2a8d212cbde1@gmail.com>
Date: Sat, 9 Nov 2024 01:31:51 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, sd@...asysnail.net,
 Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: 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.hunter@...il.com
> Signed-off-by: Antonio Quartulli <antonio@...nvpn.net>

[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@...nvpn.net>
> + *		Antonio Quartulli <antonio@...nvpn.net>
> + */
> +
> +#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