On Thursday, November 10, 2016 4:01:20 PM CET David Sommerseth wrote:
> On 28/10/16 18:42, Heiko Hund wrote:
> >
> > + 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.
The idea was that struct argv is usually very short lived. Some callers do not
have a gc and would need to create one just for using struct argv, others have
one and it is rather long lived, so it would hang on to the memory much
longer than needed.
> 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().
The difference between argv_str and argv_msg is that the latter one doesn't
pass the gc_alloced string to the calling scope. In that case the caller shoul
take care of when the string is freed.
> 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).
Thanks for taking the time to review this. Let's do another round for 5-7.
Cheers
Heiko
------------------------------------------------------------------------------
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
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
