On Tue, Jan 10, 2017 at 10:35:10PM +0100, Steffan Karger wrote: > Hi, > > In general this looks good. Some remarks inline. Please bear with me, > we're almost there ;)
No worries :) and thanks for taking some time to review my patch!
[CUT]
> > + if (!options->extra_certs_file)
> > + {
> > + errs |= check_file_access(CHKACC_FILE, options->extra_certs_file,
> > R_OK,
> > + "--extra-certs");
> > + }
>
> ... more add ifs follow.
>
> I don't particularly like all these ifs. What about adding a wrapper
> function like check_file_access_inline(bool inline, int flags, ...),
> containing basically "if(!inline) check_file_access(flags, ...)", so
> that we will here still have a nice 'check_file_access*()'-sled?
>
> (Thanks to Adriaan for the suggestion.)
Yeah, I liked the idea.
[CUT]
> > @@ -5154,7 +5194,8 @@ add_option(struct options *options,
> > {
> > options->plugin_list = plugin_option_list_new(&options->gc);
> > }
> > - if (!plugin_option_list_add(options->plugin_list, &p[1],
> > &options->gc))
> > + if (!plugin_option_list_add(options->plugin_list, &p[1], is_inline,
> > + &options->gc))
>
> Can we inline a plugin? I wouldn't think so, but I've been surprised by
> our option parser before ;-) (Arne or David might know this.)
Personally I don't know :) the parser logic seems so generic that I had the
feeling everything could be inline'd :D
> If not,
> you don't need to add the is_inline argument to
> plugin_option_list_add(), but just add a 'false' when someone down the
> chain calls make_extended_arg_array().
I agree with this - it is the right thing to do.
[CUT]
> > @@ -6496,7 +6531,7 @@ add_option(struct options *options,
> > else if (streq(p[0], "push") && p[1] && !p[2])
> > {
> > VERIFY_PERMISSION(OPT_P_PUSH);
> > - push_options(options, &p[1], msglevel, &options->gc);
> > + push_options(options, &p[1], is_inline, msglevel, &options->gc);
>
> Same as with 'plugin', I don't think we can inline a 'push' option?
My understanding is that the code allows for push-options to be inline'd, but
right now there is no push-option that can be used that way. Am I wrong?
[CUT]
> > @@ -1209,11 +1204,12 @@ tls_ctx_load_ca(struct tls_root_ctx *ctx, const
> > char *ca_file,
> > {
> > crypto_msg(M_WARN,
> > "Cannot load CA certificate file %s
> > (entry %d did not validate)",
> > - np(ca_file), added);
> > +
> > + print_if_inline(ca_file,
> > ca_file_inline),
> > + added);
>
> Indenting looks off.
Thanks for spotting. Will fix.
Cheers,
--
Antonio Quartulli
signature.asc
Description: Digital signature
------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
