On Sun, Jan 15, 2017 at 10:36:24AM +0100, Steffan Karger wrote: > > diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c > > index 944f4c5..9a7745f 100644 > > --- a/src/openvpn/crypto.c > > +++ b/src/openvpn/crypto.c > > @@ -36,6 +36,7 @@ > > #include "crypto.h" > > #include "error.h" > > #include "integer.h" > > +#include "misc.h" > > Adding a dependency to misc.o breaks the unit tests. Just try to run > 'make check' now (after initializing the cmocka submodule). > > I see that this added dependency is because you added > 'print_if_inline()' in misc.c. It seems to be used only to print key > filenames though. What about renaming it to print_key_filename() and > moving it to crypto.c?
Yeah, makes sense. I'll move it to crypto.c
>
> > +/*
> > + * A wrapper for check_file_access_chroot() that returns false immediately
> > if
> > + * the file is inline (and therefore there is no access to check)
> > + */
> > +static bool
> > +check_file_access_chroot_inline(bool is_inline, const char *chroot,
> > + const int type, const char *file,
> > + const int mode, const char *opt)
> > +{
> > + if (is_inline)
> > + {
> > + return false;
> > + }
> > +
> > + return check_file_access_chroot(chroot, type, file, mode, opt);
> > +}
> > +
> > +/*
> > + * A wrapper for check_file_access() that returns false immediately if the
> > file
> > + * is inline (and therefore there is no access to check)
> > + */
> > +static bool
> > +check_file_access_inline(bool is_inline, const int type, const char *file,
> > + const int mode, const char *opt)
> > +{
> > + if (is_inline)
> > + {
> > + return false;
> > + }
> > +
> > + return check_file_access(type, file, mode, opt);
> > +}
>
> If you start these with /**, rather than /*, they will end up nicely in
> the doxygen.
Yeah .. I usually do it, but this time I forgot :) will fix that.
>
> > static bool
> > -check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
> > +check_inline_file(struct in_src *is, char *p[], bool *is_inline,
> > + struct gc_arena *gc)
> > {
> > bool ret = false;
> > +
> > + *is_inline = false;
> > if (p[0] && !p[1])
> > {
> > char *arg = p[0];
> > if (arg[0] == '<' && arg[strlen(arg)-1] == '>')
> > {
> > struct buffer close_tag;
> > + *is_inline = true;
> > arg[strlen(arg)-1] = '\0';
> > p[0] = string_alloc(arg+1, gc);
> > - p[1] = string_alloc(INLINE_FILE_TAG, gc);
> > close_tag = alloc_buf(strlen(p[0]) + 4);
> > buf_printf(&close_tag, "</%s>", p[0]);
> > - p[2] = read_inline_file(is, BSTR(&close_tag), gc);
> > - p[3] = NULL;
> > + p[1] = read_inline_file(is, BSTR(&close_tag), gc);
> > + p[2] = NULL;
> > free_buf(&close_tag);
> > ret = true;
> > }
>
> This function now returns the same value as is set to *is_line. I think
> we should just use the return value. (For some reason the return value
> wasn't used anywhere previously, but let's just do so now.)
eheh nice catch. I will remove the parameter and re-use the return value.
>
> > static void
> > add_option(struct options *options,
> > char *p[],
> > + bool is_inline,
> > const char *file,
> > int line,
> > const int level,
> > @@ -4506,6 +4561,7 @@ read_config_file(struct options *options,
> > struct env_set *es)
> > {
> > const int max_recursive_levels = 10;
> > + bool is_inline;
>
> is_inline is only used below, in the if(parse_line()) scope. I think it
> should be declared in that scope.
oh ok. I am not used to that, so I completely missed it. will do!
>
> > FILE *fp;
> > int line_num;
> > char line[OPTION_LINE_SIZE+1];
> > @@ -4544,8 +4600,10 @@ read_config_file(struct options *options,
> > if (parse_line(line + offset, p, SIZE(p), file, line_num,
> > msglevel, &options->gc))
> > {
> > bypass_doubledash(&p[0]);
> > - check_inline_file_via_fp(fp, p, &options->gc);
> > - add_option(options, p, file, line_num, level,
> > msglevel, permission_mask, option_types_found, es);
> > + check_inline_file_via_fp(fp, p, &is_inline,
> > &options->gc);
> > + add_option(options, p, is_inline, file, line_num,
> > level,
> > + msglevel, permission_mask,
> > option_types_found,
> > + es);
> > }
> > }
> > if (fp != stdin)
> > @@ -4578,6 +4636,7 @@ read_config_string(const char *prefix,
> > char line[OPTION_LINE_SIZE];
> > struct buffer multiline;
> > int line_num = 0;
> > + bool is_inline;
> > buf_set_read(&multiline, (uint8_t *)config, strlen(config));
> >
> > @@ -4589,8 +4648,9 @@ read_config_string(const char *prefix,
> > if (parse_line(line, p, SIZE(p), prefix, line_num, msglevel,
> > &options->gc))
> > {
> > bypass_doubledash(&p[0]);
> > - check_inline_file_via_buf(&multiline, p, &options->gc);
> > - add_option(options, p, prefix, line_num, 0, msglevel,
> > permission_mask, option_types_found, es);
> > + check_inline_file_via_buf(&multiline, p, &is_inline,
> > &options->gc);
> > + add_option(options, p, is_inline, prefix, line_num, 0,
> > msglevel,
> > + permission_mask, option_types_found, es);
> > }
> > CLEAR(p);
> > }
>
> Same as above, move is_inline to the scope where it's used.
copy that!
>
> > --- a/src/openvpn/plugin.c
> > +++ b/src/openvpn/plugin.c
> > @@ -160,13 +160,13 @@ plugin_option_list_new(struct gc_arena *gc)
> > return ret;
> > }
> >
> > -bool
> > -plugin_option_list_add(struct plugin_option_list *list, char **p, struct
> > gc_arena *gc)
> > +bool plugin_option_list_add(struct plugin_option_list *list, char **p,
> > + struct gc_arena *gc)
>
> The return type should stay on it's own line.
>
argh, my bad again!
Will fix these minor glitches and send v4!
Thank you for the feedback!
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
