On 26/09/17 00:22, Steffan Karger wrote:
> 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?I personally like that. It somehow brings in the concept of 'ownership' and avoids introducing faulty code (we'd get warned as you said above). Cheers, -- Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ 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
