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



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!


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]