Re: [RFC PATCH v3 3/3] platform: add support for WireGuard links

[Beniamino Galvani <bgalvani redhat com>]

On Sun, Jun 17, 2018 at 12:34:58PM +0100, Javier Arteaga wrote:
> Add support for a new wireguard link type to the platform code. For now
> this only covers querying existing links via genetlink and parsing them
> into platform objects.
> ---
>
> Notes:
>     Changes in v2->v3:
>     
>     * Changed peers/allowedips from CList to arrays-of-structs
>       (GArray wrappers are used during genl parsing stage to handle reallocs)
>     * Wrote proper cmp/hash methods for peer objects
>     * Split off two patches not really specific to WireGuard
>     * Corrected WireGuard uapi identifier names
>     * Addressed a few other comments on v2
>
>  libnm-core/nm-core-types-internal.h |  25 +++
>  src/nm-types.h                      |   2 +
>  src/platform/nm-linux-platform.c    | 315 ++++++++++++++++++++++++++++
>  src/platform/nm-platform.c          | 124 +++++++++++
>  src/platform/nm-platform.h          |  15 ++
>  src/platform/nmp-object.c           | 219 +++++++++++++++++++
>  src/platform/nmp-object.h           |  10 +
>  7 files changed, 710 insertions(+)
>
> diff --git a/libnm-core/nm-core-types-internal.h b/libnm-core/nm-core-types-internal.h
> index 4d43aaf45..f95652fa2 100644
> --- a/libnm-core/nm-core-types-internal.h
> +++ b/libnm-core/nm-core-types-internal.h
> @@ -31,6 +31,31 @@ typedef struct {
>       guint32 to;
>  } NMVlanQosMapping;
>  
> +typedef struct {
> +     NMIPAddr ip;
> +     guint8 family;
> +     guint8 cidr;

Just a nitpick, feel free to ignore, but perhaps 'mask' would be a
better name than 'cidr'.

> [...]
> @@ -7103,6 +7416,8 @@ constructed (GObject *_object)
>       g_assert (!nle);
>       _LOGD ("Generic netlink socket established: port=%u, fd=%d", nl_socket_get_local_port (priv->genlh), 
> nl_socket_get_fd (priv->genlh));
>  
> +     priv->wireguard_family_id = _support_genl_family (priv->genlh, "wireguard");
> +

Since the genl family id is already determined when needed (i.e. in
wireguard_get_link_properties()), I don't think we need to do it here
too.

> +void
> +nm_platform_lnk_wireguard_hash_update (const NMPlatformLnkWireguard *obj, NMHashState *h)
> +{
> +     nm_hash_update_vals (h,
> +                          obj->private_key,
> +                          obj->public_key,
> +                          obj->listen_port,
> +                          obj->fwmark);
> +}

This gives the following compile error here:

  CC       src/platform/src_libNetworkManagerBase_la-nm-platform.lo
In file included from ./shared/nm-default.h:292,
                 from src/platform/nm-platform.c:21:
src/platform/nm-platform.c: In function ‘nm_platform_lnk_wireguard_hash_update’:
./shared/nm-utils/nm-hash-utils.h:121:57: error: initialization of ‘unsigned char’ from ‘const guint8 *’ {aka 
‘const unsigned char *’} makes integer from pointer without a cast [-Werror=int-conversion]
 #define _NM_HASH_COMBINE_VALS_val_x_4( y, ...)  ._v4  = (y), _NM_HASH_COMBINE_VALS_val_x_3  (__VA_ARGS__)

because private and public keys are guint8[]; I think you should use
nm_hash_update() for them.


The rest LGTM, thanks.

Beniamino

Attachment: signature.asc
Description: PGP signature