On 28/10/16 18:42, Heiko Hund wrote: > With the private gc_arena we do not have to allocate the strings > found during parsing again, since we know the arena they are > allocated in is valid as long as the argv vector is. > > Signed-off-by: Heiko Hund <heiko.h...@sophos.com> > --- > src/openvpn/argv.c | 37 > ++++++++++++++++++------------------ > src/openvpn/argv.h | 1 + > tests/unit_tests/openvpn/test_argv.c | 21 ++++++++++++++++++++ > 3 files changed, 40 insertions(+), 19 deletions(-) > > diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c > index 664785b..9f139bb 100644 > --- a/src/openvpn/argv.c > +++ b/src/openvpn/argv.c > @@ -62,6 +62,7 @@ argv_init (struct argv *a) > a->capacity = 0; > a->argc = 0; > a->argv = NULL; > + a->gc = gc_new (); > argv_extend (a, 8); > }
Any specific reason we want to keep our own gc_arena on argv? Why not
pass an existing gc_arena pointer to the argv_new()/argv_init()
functions? I believe the majority of places argv functions are called
already have a gc_arena prepared. But I do see it may complicate the
argv_reset() function.
> + const char *args[] = {
> + "good_tools_have_good_names_even_though_it_might_impair_typing",
> + "--long-opt=looooooooooooooooooooooooooooooooooooooooooooooooong",
> + "--long-cat=loooooooooooooooooooooooooooooooooooooooooooooooooooonger",
Great examples! :)
> +
> "file_with_very_descriptive_filename_that_leaves_no_questions_open.jpg.exe"
And this made me laugh! Leaves absolutely no open questions ;)
When looking over the final argv.c file, I also wonder about this:
const char *
argv_str (const struct argv *a, struct gc_arena *gc, const unsigned int
flags)
{
return print_argv ((const char **)a->argv, gc, flags);
}
Why not use our own internal gc_arena? Especially when considering the
current callers:
void
argv_msg (const int msglev, const struct argv *a)
{
struct gc_arena gc = gc_new ();
msg (msglev, "%s", argv_str (a, &gc, 0));
gc_free (&gc);
}
The situation is identical for argv_msg_prefix().
AFAICS, struct argv objects doesn't live very long, so it would be
minimal impact on the overall memory usage. At least when we have our
own internal gc_arena. Using a more global gc_arena (provided via
argv_new()/argv_init()) makes the object live somewhat longer. But even
most of those places the gc_arenas doesn't live very much longer.
The overall impression I have is that this patch-set is truly valuable
and does important clean-ups and fixes a lot of ambiguity and odd
behaviours. And getting proper unit tests on top of it all is truly
great! But there's still some improvements needed in the last 3 patches
(patch 5, 6 and 7).
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP 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
