Hi,
On 23/01/2019 03:22, Arne Schwabe wrote:
> Am 30.12.18 um 12:28 schrieb Antonio Quartulli:
>> From: Robin Tarsiger <r...@dasyatidae.com>
>>
>> This new transport protocol is used to tell the core code that traffic
>> should not be directly processed, but should rather be rerouted to a
>> transport plugin. It is basically an abstraction as it does not say tell
>> the code how to process the data, but simply forces its redirection to
>> the external code.
>>
>> Signed-off-by: Robin Tarsiger <r...@dasyatidae.com>
>> [anto...@openvpn.net: refactored commits, restyled code]
>> ---
>> src/openvpn/forward.c | 5 ++
>> src/openvpn/socket.c | 146 ++++++++++++++++++++++++++++++++++++++--
>> src/openvpn/socket.h | 70 +++++++++++++++++++
>> src/openvpn/transport.h | 5 ++
>> 4 files changed, 222 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
>> index 0a90fff0..a7092c7e 100644
>> --- a/src/openvpn/forward.c
>> +++ b/src/openvpn/forward.c
>> @@ -2150,6 +2150,11 @@ io_wait_dowork(struct context *c, const unsigned int
>> flags)
>> {
>> int i;
>> c->c2.event_set_status = 0;
>> +#ifdef ENABLE_PLUGIN
>> + c->c2.event_set_status |=
>> + (socket_indirect_pump(c->c2.link_socket, esr, &status)
>> & 3)
>
> The & 3 looks like some defined should be used instead.
I think this is simply about fetching the last two bits..not much fuzz
here..this is mostly because of how event_set_status works. But will see
if I find a meaningful name.
>
>
>
>> if (addr->ai_protocol == IPPROTO_UDP || addr->ai_socktype == SOCK_DGRAM)
>> {
>> sock->sd = create_socket_udp(addr, sock->sockflags);
>> @@ -2279,7 +2320,11 @@ link_socket_init_phase2(struct link_socket *sock,
>> }
>>
>> /* If socket has not already been created create it now */
>> - if (sock->sd == SOCKET_UNDEFINED)
>> + if (sock->sd == SOCKET_UNDEFINED
>> +#ifdef ENABLE_PLUGIN
>> + && !sock->indirect
>> +#endif
>> + )
>> {
>> /* If we have no --remote and have still not figured out the
>> * protocol family to use we will use the first of the bind */
>> @@ -2300,7 +2345,11 @@ link_socket_init_phase2(struct link_socket *sock,
>> }
>>
>> /* Socket still undefined, give a warning and abort connection */
>> - if (sock->sd == SOCKET_UNDEFINED)
>> + if (sock->sd == SOCKET_UNDEFINED
>> +#ifdef ENABLE_PLUGIN
>> + && !sock->indirect
>> +#endif
>> + )
>
> Better use the inline funtcion proto_is_indirect (or similar
> sock_is_indirect) that always returns false here and ifdef in header
> than to add the ifdefs inline in the code.
agreed! I am all for that! will change
>
>
>> bool
>> @@ -3167,6 +3236,10 @@ proto_is_net(int proto)
>> bool
>> proto_is_dgram(int proto)
>> {
>> + if (proto_is_indirect(proto))
>> + {
>> + return true;
>> + }
>> return proto_is_udp(proto);
>> }
>
> I think here need to be a good explaination why indirect is dgram but
> not udp and also proto_is_dgram and proto_is_udp need to get some
> comment to explain their difference in usage as this is now different.
I wouldn't say is_udp and is_dgram are now different, but is_dgram has
been extended to account for the indirect case as well.
>
>>
>> @@ -3301,6 +3374,18 @@ proto_remote(int proto, bool remote)
>> return "TCPv4_CLIENT";
>> }
>>
>> +#ifdef ENABLE_PLUGIN
>> + if (proto == PROTO_INDIRECT)
>> + {
>> + /* FIXME: the string reported here should match the actual transport
>> + * protocol being used, however in this function we have no
>> knowledge of
>> + * what protocol is exactly being used by the transport-plugin.
>> + * Therefore we simply return INDIRECT for now.
>> + */
>> + return "INDIRECT";
>> + }
>> +#endif
>
> From reading the code this function is only used in creating the string
> in options_string which needs to match between peers. As the plugin
> emulates UDP/DGRAM behaviour I think we should instead return here the
> same as a real UDP protocol.
well, the problem here is that if I use transport-plugin1 and you use
transport-plugin2 these strings will match between client and server
even though they should not (because the protocols being used are
actually different).
I don't think INDIRECT is UDP only. That is just how it prefers to be
treated, but could be non-UDP as well, I think. (need to double check)
>
>>
>> +#ifdef ENABLE_PLUGIN
>> +
>> +int link_socket_read_indirect(struct link_socket *sock,
>> + struct buffer *buf,
>> + struct link_socket_actual *from);
>> +
>> +int link_socket_write_indirect(struct link_socket *sock,
>> + struct buffer *buf,
>> + struct link_socket_actual *from);
>> +
>> +bool proto_is_indirect(int proto);
>> +
>
> This prototype definition looks a bit weird here. I see not reason why
> the real proto_is_indirect cannot be here
you mean the proto_is_indirect() implementation should be here in the
header? Why?
>
>> +#else /* ifdef ENABLE_PLUGIN */
>> +
>> +static int
>> +link_socket_read_indirect(struct link_socket *sock,
>> +
>> +static int
>> +link_socket_write_indirect(struct link_socket *sock,
>
>> +static bool
>> +proto_is_indirect(int proto)
>
> I think functions implemented in the header should have the inline
> keyword. At least the rest of the header files do it that way.
yeah they should. will fix that.
>
>
> Arne
>
--
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
