On 26/11/2018 22:44, Arne Schwabe wrote: > Am 11.10.18 um 20:41 schrieb Antonio Quartulli: >> iproute2 is the first user of the new networking API and >> its one of the two currently supported functionalities on >> Linux (the other being net-tools). >> >> This patch simply copies the current code from tun.c/route.c >> to networking_ip.c without introducing any funcional >> change to the code. >> > > I skipped the first patch and added notes about the proposed API in this > patch. > >> create mode 100644 src/openvpn/networking_ip.h >> >> diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am >> index 8afc4146..3caad17d 100644 >> --- a/src/openvpn/Makefile.am >> +++ b/src/openvpn/Makefile.am >> @@ -80,7 +80,7 @@ openvpn_SOURCES = \ >> mtu.c mtu.h \ >> mudp.c mudp.h \ >> multi.c multi.h \ >> - networking.h \ >> + networking_ip.c networking_ip.h networking.h \ > > As ip is not really but a linux specific tool, I think naming this > networking_linuxip.c is better as networking_ip sounds like something > generic.
I did not want to make any assumption - you never know if iproute will
later be ported to *BSD as well. But I am fine with either name.
>
>
>> ntlm.c ntlm.h \
>> occ.c occ.h \
>> openssl_compat.h \
>> diff --git a/src/openvpn/networking_ip.c b/src/openvpn/networking_ip.c
>> new file mode 100644
>> index 00000000..ae667a9c
>> --- /dev/null
>> +++ b/src/openvpn/networking_ip.c
>> @@ -0,0 +1,386 @@
>> +/*
>> + * Networking API implementation for iproute2
>> + *
>> + * Copyright (C) 2018 Antonio Quartulli <a...@unstable.cc>
>
> I think since you copied/pasted from other files this should also have
> the normal copyrights, right?
Yeah, it makes sense because in this file there is code coming from the
current implementation.
>
>> +
>> +#if defined(TARGET_LINUX) && defined(ENABLE_IPROUTE)
>> +
>> +#include "syshead.h"
>> +
>> +#include "networking.h"
>> +#include "networking_ip.h"
>> +#include "misc.h"
>> +#include "openvpn.h"
>> +#include "run_command.h"
>> +#include "socket.h"
>> +
>> +#include <stdbool.h>
>> +#include <netinet/in.h>
>> +
>> +int
>> +net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx)
>> +{
>> + ctx->es = NULL;
>> + if (c)
>> + ctx->es = c->es;
>> +
>> + return 0;
>> +}
>
> I know that calling our execve with es==NULL is valid but should not we
> aim here for consistency and always require c to be non NULL aka ASSERT(c)?
No, because this can be invoked when calling openvpn with "--show-gateway".
In that case we need to talk to the network stack but there is no context.
>
>
>> +int
>> +net_iface_up(openvpn_net_ctx_t *ctx, const char *iface, bool up)
>> +{
>> + struct argv argv = argv_new();
>> +
>> + argv_printf(&argv, "%s link set dev %s %s", iproute_path, iface,
>> + up ? "up" : "down");
>> + argv_msg(M_INFO, &argv);
>> + openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip link set
>> failed");
>> +
>> + argv_reset(&argv);
>> +
>> + return 0;
>> +}
>> +
>> +int
>> +net_iface_mtu_set(openvpn_net_ctx_t *ctx, const char *iface, uint32_t mtu)
>> +{
>> + struct argv argv = argv_new();
>> +
>> + argv_printf(&argv, "%s link set dev %s up mtu %d", iproute_path, iface,
>> + mtu);
>> + argv_msg(M_INFO, &argv);
>> + openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip link set
>> failed");
>> +
>> + return 0;
>> +}
>> +
>> +int
>> +net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface,
>> + const in_addr_t *addr, int prefixlen,
>> + const in_addr_t *broadcast)
>> +{
>> + struct argv argv = argv_new();
>> +
>> + char *addr_str = (char *)print_in_addr_t(*addr, 0, NULL);
>> + char *brd_str = (char *)print_in_addr_t(*broadcast, 0, NULL);
>> +
>> + argv_printf(&argv, "%s addr add dev %s %s/%d broadcast %s",
>> iproute_path,
>> + iface, addr_str, prefixlen, brd_str);
>> + argv_msg(M_INFO, &argv);
>> + openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add
>> failed");
>> +
>> + free(addr_str);
>> + free(brd_str);
>> +
>> + argv_reset(&argv);
>> +
>> + return 0;
>> +}
>> +
>
> I initially written a comment that this method removes the ptp variant
> of the method but later saw that there is also a ptp variant of the
> method. I would suggest to either ranme this method to
> net_addr_bcast_v4_add or have a bool ptp paramter and then call out to
> two different methods.
To me it feels like the PtP case is a special case, therefore I
preferred to keep the "standard & simple" name for the non-PtP case and
then add a special function just for PtP.
Actually this function may even be called without a broadcast address.
Mh..dunno to be honest. Maybe somebody else has an opinion?
>
>
>
>> +net_addr_v6_add(openvpn_net_ctx_t *ctx, const char *iface,
>> + const struct in6_addr *addr, int prefixlen)
>
> Same comment as for v4.
>> +net_addr_v4_del(openvpn_net_ctx_t *ctx, const char *iface,
>> + const in_addr_t *addr, int prefixlen)
>
>> +net_addr_v6_del(openvpn_net_ctx_t *ctx, const char *iface,
>> + const struct in6_addr *addr, int prefixlen)
>>
> These methods assume that removing ip on ptp and broadcast is the same
> for ptp and boradcast on all platforms, is that really a sane assumption?
>
Good point ! It is a sane assumption at least for netlink and iproute. I
am not sure about nettols.
How about Android?
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
