On 10-10-16 16:54, Lev Stipakov wrote:
> Move client-specific push options (currently peer-id and cipher) to
> separate list, which is deallocated after push_reply
> has been send. This makes sure that options fit into buf,
> not duplicated nor leak memory on renegotiation.

Feature-ACK.  Very needed refactoring.

But some very minor comments:

> - * @param o          The current connection's options
> - * @param msglevel   The message level to use when printing errors
> + * @param gc         GC arena where options are allocated
> + * @param push_list Push list containing options
> + * @param msglevel  The message level to use when printing errors
>   * @param fmt                Format string for the option

Some whitespace inconsistencies here (looks funny with ts=8).

> +prepare_push_reply (struct context *c, struct gc_arena *gc, struct push_list 
> *push_list)

> +  if (c->c2.push_ifconfig_defined && c->c2.push_ifconfig_local && 
> c->c2.push_ifconfig_remote_netmask)

> +       push_option_fmt(gc, push_list, M_USAGE, "peer-id %d", 
> tls_multi->peer_id);

These lines exceed 80 chars (and won't get much harder to read from
wrapping), so please wrap them.

I ran some smoke tests, but my jenkins broke down do didn't test very
thoroughly.  Still, the code looks very reasonable and smoke tests
succeed, so ACK if the above nitpicks are taken care of.


Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Openvpn-devel mailing list

Reply via email to