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

[Javier Arteaga <jarteaga jbeta is>]

Hi Beniamino,

Thanks for your review!

On Wed, Jun 20, 2018 at 10:39:00AM +0200, Beniamino Galvani wrote:
> > +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'.

Sure, if it parses better I'm all for it :)

> > [...]
> > @@ -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.

Yeah you're right, I'll drop it.

> > +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.

Oof. These were correctly using nm_hash_update in v2 too :/
Will fix.

(Reviewing my build settings now, it seems I lost -Werror in the process
of trying out meson, and ended up missing this warning.)

> The rest LGTM, thanks.
>
> Beniamino

Thank you!