Am 16.12.13 21:59, schrieb Gert Doering: > Hi, > > On Mon, Dec 16, 2013 at 08:10:07PM +0100, Arne Schwabe wrote: >> In 2.3 some options that were allowed only in global config before have >> been moved to connection blocks. This changes the behaviour if the variables >> were defined after connection block. This patch adds a warning to catch >> these mistakes. >> --- > Basically, I'm OK with that, just a few things... > >> diff --git a/src/openvpn/common.h b/src/openvpn/common.h >> index 2f85bec..4f32ef1 100644 >> --- a/src/openvpn/common.h >> +++ b/src/openvpn/common.h >> @@ -98,6 +98,11 @@ typedef unsigned long ptr_type; >> #define INLINE_FILE_TAG "[[INLINE]]" >> >> /* >> + * Pseudo filename for connection blocks >> + */ >> +#define CONNECTION_FILE_TAG "[CONNECTION-OPTIONS]" >> + > ... as this is only referenced once, is it really worth adding a #define > which is about as long as the string it's replacing? > Yeah. I thought it would be nice for consistency with the other special file name
>> const int line_num,
>> int msglevel,
>> struct gc_arena *gc)
>> @@ -3805,7 +3805,7 @@ read_config_string (const char *prefix,
>> {
>> bypass_doubledash (&p[0]);
>> check_inline_file_via_buf (&multiline, p, &options->gc);
>> - add_option (options, p, NULL, line_num, 0, msglevel, permission_mask,
>> option_types_found, es);
>> + add_option (options, p, prefix, line_num, 0, msglevel,
>> permission_mask, option_types_found, es);
>> }
> Is that "NULL" a generic oversight? It doesn't seem like it's needed
> for the new functionality, more like "it should have been like that",
> right? (Ah, I can see it now, we want it to not print "[CMD-LINE]"
> for these cases, right?)
Yeah. We print CMD-LINE there which is more confusing then helping. And
this function is only called by connection blocks and configs returned
by plugins. So it should be save to fix it.
>
>> CLEAR (p);
>> }
>> @@ -3925,27 +3925,43 @@ void options_string_import (struct options *options,
>>
>> #if P2MP
>>
>> -#define VERIFY_PERMISSION(mask) { if (!verify_permission(p[0], file,
>> (mask), permission_mask, option_types_found, msglevel)) goto err; }
>> +#define VERIFY_PERMISSION(mask) { if (!verify_permission(p[0], file, line,
>> (mask), permission_mask, option_types_found, msglevel, options)) goto err; }
>>
>> static bool
>> verify_permission (const char *name,
>> const char* file,
>> + int line,
>> const unsigned int type,
>> const unsigned int allowed,
>> unsigned int *found,
>> - const int msglevel)
>> + const int msglevel,
>> + struct options* options)
>> {
>> if (!(type & allowed))
>> {
>> msg (msglevel, "option '%s' cannot be used in this context (%s)",
>> name, file);
>> return false;
>> }
>> - else
>> +
>> + if (found)
>> + *found |= type;
>> +
>> +#ifndef ENABLE_SMALL
>> + /* Check if this options is allowed in connection block,
>> + * but we are currently not in a connection block
>> + * Parsing a connection block uses a temporary options struct without
>> + * connection_list
>> + */
>> +
>> + if ((type & OPT_P_CONNECTION) && options->connection_list)
>> {
>> - if (found)
>> - *found |= type;
>> - return true;
>> + if (file)
>> + msg (M_WARN, "Option '%s' in %s:%d is ignored by previous <connection>
>> blocks ", name, file, line);
>> + else
>> + msg (M_WARN, "Option '%s' is ignored by previous <connection> blocks",
>> name);
>> }
>> +#endif
>> + return true;
>> }
> OK :-)
>
> (This is actually not changing the "if (*found)..." logic, it just looks
> that way due to the way git diff sometimes produces patches where
> whitespace changes close to code additions...)
:-)
Arne
signature.asc
Description: OpenPGP digital signature
