Hi,
On 21-09-17 23:03, David Sommerseth wrote:
> On 15/09/17 08:39, Steffan Karger wrote:
>> close_instance() tries to remove the file in c2.pf.filename, but that only
>> works if we actually set that if we fail. So, set that filename as soon
>> as we know we've created the file.
>>
>> Signed-off-by: Steffan Karger <steffan.kar...@fox-it.com>
>> ---
>> v2: As suggested by Antionio, get rid of local 'gc' and 'file' vars.
>>
>> src/openvpn/pf.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
>> index 5fe1734..7479347 100644
>> --- a/src/openvpn/pf.c
>> +++ b/src/openvpn/pf.c
>> @@ -618,19 +618,18 @@ pf_load_from_buffer_list(struct context *c, const
>> struct buffer_list *config)
>> void
>> pf_init_context(struct context *c)
>> {
>> - struct gc_arena gc = gc_new();
>> #ifdef PLUGIN_PF
>> if (plugin_defined(c->plugins, OPENVPN_PLUGIN_ENABLE_PF))
>> {
>> - const char *pf_file = create_temp_file(c->options.tmp_dir, "pf",
>> &gc);
>> - if (pf_file)
>> + c->c2.pf.filename = create_temp_file(c->options.tmp_dir, "pf",
>> + &c->c2.gc);
>
> Patch looks good. But it introduces a new compile warning.
>
> pf.c: In function ‘pf_init_context’:
> pf.c:624:27: warning: assignment discards ‘const’ qualifier from pointer
> target type [enabled by default]
> c->c2.pf.filename = create_temp_file(c->options.tmp_dir, "pf",
>
> I'm pondering if we need create_temp_file() to actually return
> a const char * - wouldn't just a plain char * be enough?
Good point. But I think it's best to return a const char * here,
because the memory is allocated through a gc and therefore should also
be free'd through the gc. The const modifier prevents freeing the data
without a warning (as long as you keep smacking me for discarding const
qualifiers, or we add a -Werror compile flag ;-) ).
> The alternative is to cast the const away here; but that just
> feels too hacky in this code path.
Or change c->c2.filename to be const char *. I think that's the way to
go here. And possibly reject a NULL gc argument in create_temp_file(),
because that would make the function leak memory...
What do you think?
-Steffan
------------------------------------------------------------------------------
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
