Hi Thomas,
Thanks a lot for your feedback! Comments inline.
On Wed, Mar 14, 2018 at 3:15 PM, Thomas Haller <thal...@redhat.com> wrote:
[...]
>> + NMP_OBJECT_TYPE_LNK_WIREGUARD,
>> +
>> + NMP_OBJECT_TYPE_WIREGUARD_PEER,
>> + NMP_OBJECT_TYPE_WIREGUARD_ALLOWEDIP,
>
> You ask whether peer/allowed-ip should be their own NMPObject
> implementations. Maybe, but it's not stringend.
>
[...]
> I tend to think they should not be own objects. If they are their own
> NMPObject instances, they maybe should not be linked via an embedded
> CList, beause it means they cannot be shared, which is a bit unusual
> for these objects.
>
> When receiving these objects, you also commonly get them all in one go,
> like for the VLAN egress-qos-map. That is another reason why it doesn't
> make much sense to have them individual objects. You can just allocate
> one large array and put all the parsed structs inside.
That looks simpler. I'll follow the example of egress-qos-map for v2.
>> + allowedip->family = tba[IFLA_WG_ALLOWEDIP_FAMILY] ?
>> nla_get_u16 (tba[IFLA_WG_ALLOWEDIP_FAMILY]) : 0;
>> +
>> + addr_len = (allowedip->family == AF_INET)
>
> check that the family is at least AF_INET6, otherwise error out.
Will fix.
>> +static NMPObject *
>> +_parse_lnk_wireguard (const char *kind, const char *ifname)
>> +{
>> + nm_auto_nmpobj NMPObject *obj = NULL;
>> + NMPObject *obj_result = NULL;
>> + struct nl_sock *sock;
>> + nm_auto_nlmsg struct nl_msg *msg = NULL;
>> + struct nl_cb cb = {
>> + .valid_cb = _wireguard_parse_getdevice,
>> + };
>> + static int family_id;
>> +
>> + if (!nm_streq0 (kind, "wireguard"))
>> + goto err;
>> +
>> + sock = nl_socket_alloc ();
>> + if (!sock)
>> + goto err;
>> +
>> + if (nl_connect (sock, NETLINK_GENERIC))
>
> I think the genl socket should be cached and re-used by
> NMLinuxPlatform. Likewise, family_id should be cached in
> NMLinuxPlatformPrivate.
>
I'll look into it.
>
>> + goto err_socket;
>
> It's a bit odd that _parse_lnk_wireguard() opens a genl socket to
> request additional data. Commonly the entire parsing of
> _new_from_nl_link() is only supposed to get information that is present
> in the netlink message. They only thing that they might do, is look
> into the platform cache, and re-use information from there, for example
> at
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/platform/nm-linux-platform.c?id=167e42a87e97ed7fb26a4263c22f1774716ac51b#n785
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/platform/nm-linux-platform.c?id=167e42a87e97ed7fb26a4263c22f1774716ac51b#n1833
>
> Basically, for every RTM_NEWLINK event, it re-fetches all wireguard
> information from genetlink. That seems a bit heavy. Do we get a
> notification on (ge)netlink when something about this interface
> changes?
>
> Otherwise, you could just look into the platform cache, see that the
> information is already, and re-use it from there.
Yeah it does feel like an abuse of _new_from_nl_link().
I don't yet know if there are any options to subscribe to link changes.
I'll keep digging.
>> + if (!family_id)
>> + family_id = genl_ctrl_resolve (sock, "wireguard");
>> + if (family_id <= 0)
>> + goto err_socket;
>> +
>> + msg = nlmsg_alloc ();
>> +
>> + if (!genlmsg_put (msg, NL_AUTO_PORT, NL_AUTO_SEQ, family_id,
>> + 0, NLM_F_DUMP, WG_CMD_GET_DEVICE, 1))
>> + goto err_socket;
>> +
>> + if (nla_put_string (msg, IFLA_WG_DEVICE_IFNAME, ifname) < 0)
>
> can we avoid lookup by ifname? The ifname can be changed, only ifindex
> is the unchangable, unique ID (mostly, not really either :( )
Checking WireGuard code now - yes we can lookup by ifindex.
I'll make that change.
>> +const char *
>> +nm_platform_lnk_wireguard_to_string (const NMPlatformLnkWireguard
>> *lnk, char *buf, gsize len)
>> +{
>> + gchar *public_b64;
>> + if (!nm_utils_to_string_buffer_init_null (lnk, &buf, &len))
>> + return buf;
>> +
>> + public_b64 = g_base64_encode (lnk->public_key, sizeof (lnk-
>> >public_key));
>> +
>> + g_snprintf (buf, len,
>> + "wireguard "
>> + "public_key %s "
>> + "private_key (hidden) "
>> + "listen_port %u "
>> + "fwmark 0x%x",
>> + public_b64,
>> + lnk->listen_port,
>> + lnk->fwmark);
>> +
>> + g_free (public_b64);
>
> don't explicitly free. Use one of the gs_* or nm_auto_* macros.
> In this case, gs_free.
OK.
> Commonly, the to-string methods should print *all* aspects of the
> object. It's right to hide the private-key, I am talking about the
> Peers/AllowedIPs.
>
> The sole purpose of the to-string methods if for debugging. Currently,
> the peers/allowed-ips are never printed, maybe they should.
>
> Note also that nmp_object_to_string() has an arguemnt to_string_mode,
> for printing common fields (NMP_OBJECT_TO_STRING_PUBLIC) vs. all
> (NMP_OBJECT_TO_STRING_ALL).
> I think at least for NMP_OBJECT_TO_STRING_ALL, NMPObjectLnkWireguard
> should print all peers/allowed-ips. This means to implement
> "cmd_obj_to_string".
> See
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/platform/nmp-object.c?id=167e42a87e97ed7fb26a4263c22f1774716ac51b#n666
Sure. What about:
NMP_OBJECT_TO_STRING_PUBLIC:
* Device: public key, port, fwmark, peers...
* Each peer: public key, endpoint, keepalive interval, allowed IPs..
* Each allowed IP: CIDR notation address
NMP_OBJECT_TO_STRING_ALL:
* All of the above plus every single field (including all stats).
>> +const char *
>> +nm_platform_wireguard_peer_to_string (const NMPlatformWireguardPeer
>> *peer, char *buf, gsize len)
>> +{
>> + gchar *public_b64;
>> + char s_endpoint[NI_MAXHOST + NI_MAXSERV + sizeof("endpoint
>> []:") + 1];
>> +
>> + if (!nm_utils_to_string_buffer_init_null (peer, &buf, &len))
>> + return buf;
>> +
>> + if (peer->endpoint.addr.sa_family == AF_INET || peer-
>> >endpoint.addr.sa_family == AF_INET6) {
>> + char host[NI_MAXHOST];
>> + char service[NI_MAXSERV];
>> + socklen_t addr_len = 0;
>> +
>> + if (peer->endpoint.addr.sa_family == AF_INET)
>> + addr_len = sizeof (struct sockaddr_in);
>> + else if (peer->endpoint.addr.sa_family == AF_INET6)
>> + addr_len = sizeof (struct sockaddr_in6);
>> + if (!getnameinfo (&peer->endpoint.addr, addr_len,
>> host, sizeof(host), service, sizeof(service), NI_DGRAM |
>> NI_NUMERICSERV | NI_NUMERICHOST)) {
>
> I don't think we should resolve the addresses just for logging.
> Just convert them to string with nm_utils_inet_ntop().
I understood getnameinfo() with NI_NUMERIC* didn't do resolution.
Anyhow, I can use that utils function if you prefer.
>> +typedef struct {
>> + guint8 private_key[NM_WG_PUBLIC_KEY_LEN];
>> + guint8 public_key[NM_WG_PUBLIC_KEY_LEN];
>> + guint16 listen_port;
>> + guint32 fwmark;
>> +
>> + CList peers_lst_head;
>
> The public part of the NMPObjects (NMPlatformLnkWireguard vs.
> NMPObjectLnkWireguard) is supposed to be copyable directly. That means,
> it cannot have pointers (except for example NMPlatformLink.kind, which
> is however a static string, so it's safe to copy).
>
> This means, the CList part must go into NMPObjectLnkWireguard.
>
>
> Together with I said above about nm_platform_lnk_wireguard_to_string()
> to print all fields, it means, the list of peers/allowed-ips is not
> accessible to NMPlatformLnkWireguard, so there is nothing to print.
> But it certainly should implement cmd_obj_to_string().
Got it - I'll go the NMPObjectLnkVlan route instead as you suggest.
>> + [NMP_OBJECT_TYPE_LNK_WIREGUARD - 1] = {
>> + .parent =
>> DEDUP_MULTI_OBJ_CLASS_INIT(),
>> + .obj_type =
>> NMP_OBJECT_TYPE_LNK_WIREGUARD,
>> + .sizeof_data = sizeof
>> (NMPObjectLnkWireguard),
>> + .sizeof_public = sizeof
>> (NMPlatformLnkWireguard),
>> + .obj_type_name = "wireguard",
>> + .lnk_link_type =
>> NM_LINK_TYPE_WIREGUARD,
>> + .cmd_plobj_to_string = (const char
>> *(*) (const NMPlatformObject *obj, char *buf, gsize len))
>
> the types that are put inside the platform cache (like
> NMPObjectLnkWireguard), they must implement equality/hash operators.
>
> NMPObjectLnkWireguard is not unlike NMPObjectLnkVlan. See what
> functions are implemented there.
Will do.
(Actually had these functions in before, following the macsec example,
but then I thought I'd just make the patch simpler until I was sure what
the wireguard data structures would look like.)
On Wed, Mar 14, 2018 at 5:20 PM, Thomas Haller <thal...@redhat.com> wrote:
[...]
>> +static NMPObject *
>> +_parse_lnk_wireguard (const char *kind, const char *ifname)
>> +{
>> + nm_auto_nmpobj NMPObject *obj = NULL;
>> + NMPObject *obj_result = NULL;
>> + struct nl_sock *sock;
>> + nm_auto_nlmsg struct nl_msg *msg = NULL;
>> + struct nl_cb cb = {
>> + .valid_cb = _wireguard_parse_getdevice,
>> + };
>> + static int family_id;
>
> Hi,
>
> all the "parsing" done here is basically requesting/querying everyhing
> via genl.
>
> It would be really nice, to only request new data when necessary, and
> otherwise keep the current from the cache.
>
> But how to notice changes?
>
> Maybe the wireguard netlink API should expose a version id both on rtnl
> and genl. Then, if we receive a new (different) version id on rtnl, we
> know we need to refetch everything.
>
> Or does the wireguard API already provide some functionality about
> this? Should we instead register to signals on genl? That would be
> doable, but cumbersome.
Not sure, I'd wait for Jason's answer on this one. I can see a
device_update_gen counter within struct wireguard_device, but I don't
think that is meant to be reported via rtnl.
Again, thanks for your time!
_______________________________________________
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list
