Hi,
On 26/11/2018 23:14, Arne Schwabe wrote:
> Am 11.10.18 um 20:41 schrieb Antonio Quartulli:
>> This patch introduces a tiny netlink interface, optimized
>> for the openvpn use case.
>>
>> It basically exposes all those operations that are currently
>> handled by directly calling the /sbin/ip command (or even
>> ifconfig/route, if configured).
>>
>> By using netlink, openvpn won't need to spawn new processes
>> when configuring the tun interface or routes.
>> This new approach will also allow openvpn to be granted
>> CAP_NET_ADMIN and be able to properly work even though it
>> dropped the root privileges (currently handled via workarounds).
>>
>> By moving this logic into the sitnl module, tun.c and route.c
>> also benefit from some code simplification
>
> Just FYI for the rest. The current code still has a bug handling ipv6
> and I am waiting for V3 that does not have this bug.
>
> So this is not a full review
>
>> +typedef union {
>> + in_addr_t ipv4;
>> + struct in6_addr ipv6;
>> +} inet_address_t;
>> +
>
>
>
>> +/**
>> + * Link state request message
>> + */
>> +struct sitnl_link_req {
>> + struct nlmsghdr n;
>
> I think one two sentences that netlink requires messages in a certain
> format but has no standard header that defines them so we need to define
> the message ourselves would be good here.
Ok, I can add some comments - doc is always good :)
>
>> +
>> +/* if (((int)nladdr.nl_pid != peer) || (h->nlmsg_pid !=
>> nladdr.nl_pid)
>> + || (h->nlmsg_seq != seq))
>> + {
>> + rcv_len -= NLMSG_ALIGN(len);
>> + h = (struct nlmsghdr *)((char *)h + NLMSG_ALIGN(len));
>> + msg(M_DEBUG, "%s: skipping unrelated message. nl_pid:%d
>> (peer:%d) nl_msg_pid:%d nl_seq:%d seq:%d",
>> + __func__, (int)nladdr.nl_pid, peer, h->nlmsg_pid,
>> + h->nlmsg_seq, seq);
>> + continue;
>> + }
>> +*/
>
> The "normal" style for these debug commented out parts is using #ifdef
> instead of comments.
Right. I'll check if this is really useful - if not, I will just remove it.
>
>
>> +typedef struct {
>> + int addr_size;
>> + inet_address_t gw;
>> + char iface[IFNAMSIZ];
>> +} route_res_t;
>> +
>
> This struct is in the middle of the file and I think it should either go
> along with the other struct definition in the top of the file or all
> structs should be defined before the function that they are used in first.
>
Right! Consistency is a good thing :) I'll move it to the top.
>
>> +/* used by iproute2 implementation too */
>
> I would not add that comment. Where a function is used can be inferred
> pretty easily and we don't have such comments elsewhere. And I fear this
> comment might be overlooked when iproute2 is remove in 2.6/2.7 and then
> more confusing than helpful.
Agreed. Will remove the comment.
Thanks!
Regards,
--
Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
