Hi Arne and thanks for the review! On 23/01/2019 03:03, Arne Schwabe wrote: >> >> +/* >> + * FUNCTION: openvpn_plugin_get_vtab_v1 >> + * > > It would be nice if we also use the docutils style to document the new > functions in this file rather than is this different documentation style.
I agree - will fix this.
>
>> + * This is only used for TRANSPORT plugins presently. It is called to
>> + * retrieve a vtable structure to be used for binding virtual sockets
>> + * which use the transport provided by the plugin. The selector is an
>> + * OPENVPN_VTAB constant. *size_out must be set to the size of the
>> + * structure returned.
>
> A reference to openvpn_transport_bind_vtab1 might be a good idea here.
ok, will have a look.
>
>> +/* On Windows, platform-native events to wait on are provided to OpenVPN
>> core as
>
> I think for multi line comments we use the following style
>
> /*
> * first line of text
> * last line of text
> */
>
> but not sure. Might want to double check that style.
>
as discussed on IRC it is acceptable to have multi-line comments
starting directly on the first line. So I'll keep them as they are.
> Rest of the file look fine but I also would like to see docstyle comments.
>> +#endif
>> diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
>> index 197e62ba..41cd90aa 100644
>> --- a/src/openvpn/Makefile.am
>> +++ b/src/openvpn/Makefile.am
>> @@ -119,6 +119,7 @@ openvpn_SOURCES = \
>> status.c status.h \
>> syshead.h \
>> tls_crypt.c tls_crypt.h \
>> + transport.c transport.h \
>> tun.c tun.h \
>> win32.h win32.c \
>> cryptoapi.h cryptoapi.c
>
>> +openvpn_transport_socket_t
>> +transport_bind(const struct plugin_list *plugins,
>> + const char **transport_plugin_argv, sa_family_t ai_family,
>> + struct addrinfo *bind_addresses)
>> +{
>> + openvpn_plugin_handle_t handle;
>> + openvpn_transport_args_t args;
>> + openvpn_transport_socket_t indirect;
>> + struct openvpn_transport_bind_vtab1 *vtab;
>> + struct addrinfo *cur = NULL;
>> + struct openvpn_sockaddr zero;
>> +
>> + if (!transport_prepare(plugins, transport_plugin_argv, &vtab, &handle,
>> + &args))
>> + {
>> + msg(M_FATAL, "INDIRECT: Socket bind failed: provider plugin not
>> found");
>> + }
>> +
>> + /* Partially replicates the functionality of socket_bind. No
>> bind_ipv6_only
>> + * or other such options, presently.
>> + */
>> + if (bind_addresses)
>> + {
>> + for (cur = bind_addresses; cur; cur = cur->ai_next)
>> + {
>> + if (cur->ai_family == ai_family)
>> + {
>> + break;
>> + }
>> + }
>> +
>> + if (!cur)
>> + {
>> + msg(M_FATAL, "INDIRECT: Socket bind failed: Addr to bind has no
>> %s record",
>> + addr_family_name(ai_family));
>> + }
>> + }
>> +
>> + if (cur)
>> + {
>> + indirect = vtab->bind(handle, args, cur->ai_addr, cur->ai_addrlen);
>> + }
>> + else if (ai_family == AF_UNSPEC)
>> + {
>> + msg(M_ERR, "INDIRECT: cannot bind with unspecified address family");
>> + }
>> + else
>> + {
>> + memset(&zero, 0, sizeof(zero));
>> + zero.addr.sa.sa_family = ai_family;
>> + addr_zero_host(&zero);
>> + indirect = vtab->bind(handle, args, &zero.addr.sa,
>> af_addr_size(ai_family));
>> + }
>> +
>> + if (!indirect)
>> + {
>> + msg(M_ERR, "INDIRECT: Socket bind failed");
>> + }
>> +
>> + if (vtab->freeargs)
>> + {
>> + vtab->freeargs(args);
>> + }
>> +
>> + return indirect;
>> +}
>> +
>> +struct encapsulated_event_set
>> +{
>> + struct openvpn_transport_event_set_handle handle;
>> + struct event_set *real;
>> +};
>> +
>> +#if EVENT_READ == OPENVPN_TRANSPORT_EVENT_READ \
>> + && EVENT_WRITE == OPENVPN_TRANSPORT_EVENT_WRITE
>> +#define TRANSPORT_EVENT_BITS_IDENTICAL 1
>> +#else
>> +#define TRANSPORT_EVENT_BITS_IDENTICAL 0
>> +#endif
>
> A small comment when this is identical like Win32 vs posix-y would be nice.
ok!
>
>
>> +
>> +unsigned
>> +transport_pump(openvpn_transport_socket_t indirect,
>> + struct event_set_return *esr, int *esrlen)
>> +{
>
>
> The name 'pump' is confusing, especially without comment. Why not
> transport_update_event? Something that tells what the function actually
> does.
I believe this is called such because it "injects" (hence pump) data
into the event buffer (esr). But I agree that a better name can be found.
'transport_update_event' might be ok.
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
