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
  return print_argv ((const char **)a->argv, gc, flags);

Why not use our own internal gc_arena?  Especially when considering the
current callers:

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

Attachment: 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

Reply via email to