Welcome! Log In Create A New Profile

Advanced

[PATCH] CLEANUP: pattern: Move pattern_finalize_config to post checks initialization

Posted by Nenad Merdanovic 
Signed-off-by: Nenad Merdanovic <[email protected]>
---
include/proto/pattern.h | 2 --
src/haproxy.c | 2 --
src/pattern.c | 9 ++++++++-
3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/proto/pattern.h b/include/proto/pattern.h
index 9c93db9..1a17445 100644
--- a/include/proto/pattern.h
+++ b/include/proto/pattern.h
@@ -37,8 +37,6 @@ extern void (*pat_prune_fcts[PAT_MATCH_NUM])(struct pattern_expr *);
extern struct pattern *(*pat_match_fcts[PAT_MATCH_NUM])(struct sample *, struct pattern_expr *, int);
extern int pat_match_types[PAT_MATCH_NUM];

-void pattern_finalize_config(void);
-
/* return the PAT_MATCH_* index for match name "name", or < 0 if not found */
static inline int pat_find_match_name(const char *name)
{
diff --git a/src/haproxy.c b/src/haproxy.c
index 559b481..bd7a5ca 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -809,8 +809,6 @@ static void init(int argc, char **argv)
exit(1);
}

- pattern_finalize_config();
-
err_code |= check_config_validity();
if (err_code & (ERR_ABORT|ERR_FATAL)) {
Alert("Fatal errors found in configuration.\n");
diff --git a/src/pattern.c b/src/pattern.c
index 60fe462..224e326 100644
--- a/src/pattern.c
+++ b/src/pattern.c
@@ -2463,7 +2463,7 @@ int pattern_delete(struct pattern_expr *expr, struct pat_ref_elt *ref)
/* This function finalize the configuration parsing. Its set all the
* automatic ids
*/
-void pattern_finalize_config(void)
+static int pattern_finalize_config(void)
{
int i = 0;
struct pat_ref *ref, *ref2, *ref3;
@@ -2509,4 +2509,11 @@ void pattern_finalize_config(void)
/* swap root */
LIST_ADD(&pr, &pattern_reference);
LIST_DEL(&pr);
+ return 0;
+}
+
+__attribute__((constructor))
+static void __pattern_init(void)
+{
+ hap_register_post_check(pattern_finalize_config);
}
--
2.9.3
Hi Nenad,

[ccing Thierry]

On Sun, Mar 12, 2017 at 10:00:51PM +0100, Nenad Merdanovic wrote:
> Signed-off-by: Nenad Merdanovic <[email protected]>
> ---
> include/proto/pattern.h | 2 --
> src/haproxy.c | 2 --
> src/pattern.c | 9 ++++++++-
> 3 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/include/proto/pattern.h b/include/proto/pattern.h
> index 9c93db9..1a17445 100644
> --- a/include/proto/pattern.h
> +++ b/include/proto/pattern.h
> @@ -37,8 +37,6 @@ extern void (*pat_prune_fcts[PAT_MATCH_NUM])(struct pattern_expr *);
> extern struct pattern *(*pat_match_fcts[PAT_MATCH_NUM])(struct sample *, struct pattern_expr *, int);
> extern int pat_match_types[PAT_MATCH_NUM];
>
> -void pattern_finalize_config(void);
> -
> /* return the PAT_MATCH_* index for match name "name", or < 0 if not found */
> static inline int pat_find_match_name(const char *name)
> {
> diff --git a/src/haproxy.c b/src/haproxy.c
> index 559b481..bd7a5ca 100644
> --- a/src/haproxy.c
> +++ b/src/haproxy.c
> @@ -809,8 +809,6 @@ static void init(int argc, char **argv)
> exit(1);
> }
>
> - pattern_finalize_config();
> -
> err_code |= check_config_validity();
> if (err_code & (ERR_ABORT|ERR_FATAL)) {
> Alert("Fatal errors found in configuration.\n");
> diff --git a/src/pattern.c b/src/pattern.c
> index 60fe462..224e326 100644
> --- a/src/pattern.c
> +++ b/src/pattern.c
> @@ -2463,7 +2463,7 @@ int pattern_delete(struct pattern_expr *expr, struct pat_ref_elt *ref)
> /* This function finalize the configuration parsing. Its set all the
> * automatic ids
> */
> -void pattern_finalize_config(void)
> +static int pattern_finalize_config(void)
> {
> int i = 0;
> struct pat_ref *ref, *ref2, *ref3;
> @@ -2509,4 +2509,11 @@ void pattern_finalize_config(void)
> /* swap root */
> LIST_ADD(&pr, &pattern_reference);
> LIST_DEL(&pr);
> + return 0;
> +}
> +
> +__attribute__((constructor))
> +static void __pattern_init(void)
> +{
> + hap_register_post_check(pattern_finalize_config);
> }

Are you sure about this one ? The post_check will cause the iniitialization
to be called *after* check_config_validity(). I'm not saying it's not
compatible, I just don't know if what it does is needed during the checks.
If you have already checked, I'm fine with it (but then please mention it
in the commit message).

Thanks,
Willy
Hey Willy,

On 3/13/2017 6:32 PM, Willy Tarreau wrote:
> Hi Nenad,
>
> [ccing Thierry]
>
> On Sun, Mar 12, 2017 at 10:00:51PM +0100, Nenad Merdanovic wrote:
>> Signed-off-by: Nenad Merdanovic <[email protected]>
>> ---
>> include/proto/pattern.h | 2 --
>> src/haproxy.c | 2 --
>> src/pattern.c | 9 ++++++++-
>> 3 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/proto/pattern.h b/include/proto/pattern.h
>> index 9c93db9..1a17445 100644
>> --- a/include/proto/pattern.h
>> +++ b/include/proto/pattern.h
>> @@ -37,8 +37,6 @@ extern void (*pat_prune_fcts[PAT_MATCH_NUM])(struct pattern_expr *);
>> extern struct pattern *(*pat_match_fcts[PAT_MATCH_NUM])(struct sample *, struct pattern_expr *, int);
>> extern int pat_match_types[PAT_MATCH_NUM];
>>
>> -void pattern_finalize_config(void);
>> -
>> /* return the PAT_MATCH_* index for match name "name", or < 0 if not found */
>> static inline int pat_find_match_name(const char *name)
>> {
>> diff --git a/src/haproxy.c b/src/haproxy.c
>> index 559b481..bd7a5ca 100644
>> --- a/src/haproxy.c
>> +++ b/src/haproxy.c
>> @@ -809,8 +809,6 @@ static void init(int argc, char **argv)
>> exit(1);
>> }
>>
>> - pattern_finalize_config();
>> -
>> err_code |= check_config_validity();
>> if (err_code & (ERR_ABORT|ERR_FATAL)) {
>> Alert("Fatal errors found in configuration.\n");
>> diff --git a/src/pattern.c b/src/pattern.c
>> index 60fe462..224e326 100644
>> --- a/src/pattern.c
>> +++ b/src/pattern.c
>> @@ -2463,7 +2463,7 @@ int pattern_delete(struct pattern_expr *expr, struct pat_ref_elt *ref)
>> /* This function finalize the configuration parsing. Its set all the
>> * automatic ids
>> */
>> -void pattern_finalize_config(void)
>> +static int pattern_finalize_config(void)
>> {
>> int i = 0;
>> struct pat_ref *ref, *ref2, *ref3;
>> @@ -2509,4 +2509,11 @@ void pattern_finalize_config(void)
>> /* swap root */
>> LIST_ADD(&pr, &pattern_reference);
>> LIST_DEL(&pr);
>> + return 0;
>> +}
>> +
>> +__attribute__((constructor))
>> +static void __pattern_init(void)
>> +{
>> + hap_register_post_check(pattern_finalize_config);
>> }
>
> Are you sure about this one ? The post_check will cause the iniitialization
> to be called *after* check_config_validity(). I'm not saying it's not
> compatible, I just don't know if what it does is needed during the checks.
> If you have already checked, I'm fine with it (but then please mention it
> in the commit message).

I checked and didn't see any impact to check_config_validity(). This
function assigns unique IDs to the patterns for CLI purposes and
initializes the pattern LRU cache. It is pretty much the same as
tlskeys_finalize_config() which was already moved to post checks init.

I'll resend the patch with the new commit message.

Regards,
Nenad

>
> Thanks,
> Willy
>
On Mon, 13 Mar 2017 18:54:52 +0100
Nenad Merdanovic <[email protected]> wrote:

> Hey Willy,
>
> On 3/13/2017 6:32 PM, Willy Tarreau wrote:
> > Hi Nenad,
> >
> > [ccing Thierry]
> >
> > On Sun, Mar 12, 2017 at 10:00:51PM +0100, Nenad Merdanovic wrote:
> >> Signed-off-by: Nenad Merdanovic <[email protected]>
> >> ---
> >> include/proto/pattern.h | 2 --
> >> src/haproxy.c | 2 --
> >> src/pattern.c | 9 ++++++++-
> >> 3 files changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/proto/pattern.h b/include/proto/pattern.h
> >> index 9c93db9..1a17445 100644
> >> --- a/include/proto/pattern.h
> >> +++ b/include/proto/pattern.h
> >> @@ -37,8 +37,6 @@ extern void (*pat_prune_fcts[PAT_MATCH_NUM])(struct pattern_expr *);
> >> extern struct pattern *(*pat_match_fcts[PAT_MATCH_NUM])(struct sample *, struct pattern_expr *, int);
> >> extern int pat_match_types[PAT_MATCH_NUM];
> >>
> >> -void pattern_finalize_config(void);
> >> -
> >> /* return the PAT_MATCH_* index for match name "name", or < 0 if not found */
> >> static inline int pat_find_match_name(const char *name)
> >> {
> >> diff --git a/src/haproxy.c b/src/haproxy.c
> >> index 559b481..bd7a5ca 100644
> >> --- a/src/haproxy.c
> >> +++ b/src/haproxy.c
> >> @@ -809,8 +809,6 @@ static void init(int argc, char **argv)
> >> exit(1);
> >> }
> >>
> >> - pattern_finalize_config();
> >> -
> >> err_code |= check_config_validity();
> >> if (err_code & (ERR_ABORT|ERR_FATAL)) {
> >> Alert("Fatal errors found in configuration.\n");
> >> diff --git a/src/pattern.c b/src/pattern.c
> >> index 60fe462..224e326 100644
> >> --- a/src/pattern.c
> >> +++ b/src/pattern.c
> >> @@ -2463,7 +2463,7 @@ int pattern_delete(struct pattern_expr *expr, struct pat_ref_elt *ref)
> >> /* This function finalize the configuration parsing. Its set all the
> >> * automatic ids
> >> */
> >> -void pattern_finalize_config(void)
> >> +static int pattern_finalize_config(void)
> >> {
> >> int i = 0;
> >> struct pat_ref *ref, *ref2, *ref3;
> >> @@ -2509,4 +2509,11 @@ void pattern_finalize_config(void)
> >> /* swap root */
> >> LIST_ADD(&pr, &pattern_reference);
> >> LIST_DEL(&pr);
> >> + return 0;
> >> +}
> >> +
> >> +__attribute__((constructor))
> >> +static void __pattern_init(void)
> >> +{
> >> + hap_register_post_check(pattern_finalize_config);
> >> }
> >
> > Are you sure about this one ? The post_check will cause the iniitialization
> > to be called *after* check_config_validity(). I'm not saying it's not
> > compatible, I just don't know if what it does is needed during the checks.
> > If you have already checked, I'm fine with it (but then please mention it
> > in the commit message).
>
> I checked and didn't see any impact to check_config_validity(). This
> function assigns unique IDs to the patterns for CLI purposes and
> initializes the pattern LRU cache. It is pretty much the same as
> tlskeys_finalize_config() which was already moved to post checks init.
>
> I'll resend the patch with the new commit message.


Hi, sorry for my response time. I read the code and I don't see any
suspicious point. In addition, we can take in account these points:

- The LRU are useless with the configuration check

- The pattern list id are not used by configuration, only by the cli,
so the configuration check connot fail if ID are wrongly positionned.

So, I think that this patch will not cause any problem. I think that
this initialization way is cleaner than the old function call in the
haproxy main code.

br,
Thierry.


> Regards,
> Nenad
>
> >
> > Thanks,
> > Willy
> >
Sorry, only registered users may post in this forum.

Click here to login