From 6075b06cbc08c896d9b5e865bb77516981538ffa Mon Sep 17 00:00:00 2001 From: Alexis Mousset Date: Thu, 7 Sep 2023 15:25:18 +0200 Subject: [PATCH 1/2] Added the function name to the result cache key The function cache only used the args values, which in some cases could lead to mixing results from different functions with the same arguments. Ticket: CFE-4244 Changelog: Cashed policy function results now take into account number of arguments and function name. Signed-off-by: Lars Erik Wik Co-authored-by: Alexis Mousset (cherry picked from commit 29e60a9ba05016847c3b601f469bcb5e19147960) Signed-off-by: Lars Erik Wik --- libpromises/eval_context.c | 18 ++++++- .../01_vars/02_functions/cache_name.cf | 51 +++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 tests/acceptance/01_vars/02_functions/cache_name.cf diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c index 063dd1fd384..c23ae8f07e3 100644 --- a/libpromises/eval_context.c +++ b/libpromises/eval_context.c @@ -2806,7 +2806,14 @@ bool EvalContextFunctionCacheGet(const EvalContext *ctx, return false; } - Rval *rval = FuncCacheMapGet(ctx->function_cache, args); + // The cache key is made of the function name and all args values + Rlist *args_copy = RlistCopy(args); + assert(fp != NULL); + assert(fp->name != NULL); + assert(ctx != NULL); + Rlist *key = RlistPrepend(&args_copy, fp->name, RVAL_TYPE_SCALAR); + Rval *rval = FuncCacheMapGet(ctx->function_cache, key); + RlistDestroy(key); if (rval) { if (rval_out) @@ -2832,7 +2839,14 @@ void EvalContextFunctionCachePut(EvalContext *ctx, Rval *rval_copy = xmalloc(sizeof(Rval)); *rval_copy = RvalCopy(*rval); - FuncCacheMapInsert(ctx->function_cache, RlistCopy(args), rval_copy); + + Rlist *args_copy = RlistCopy(args); + assert(fp != NULL); + assert(fp->name != NULL); + assert(ctx != NULL); + Rlist *key = RlistPrepend(&args_copy, fp->name, RVAL_TYPE_SCALAR); + + FuncCacheMapInsert(ctx->function_cache, key, rval_copy); } /* cfPS and associated machinery */ diff --git a/tests/acceptance/01_vars/02_functions/cache_name.cf b/tests/acceptance/01_vars/02_functions/cache_name.cf new file mode 100644 index 00000000000..abbace92386 --- /dev/null +++ b/tests/acceptance/01_vars/02_functions/cache_name.cf @@ -0,0 +1,51 @@ +####################################################### +# +# Test that the function result cache checks function name +# +####################################################### + +body common control +{ + inputs => { "../../default.cf.sub" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### + + +bundle agent init +{ + vars: + "agent_regex" string => ".*cf-agent.*"; +} + +####################################################### + +bundle common test +{ + meta: + "description" -> { "CFE-4244" } + string => "Test that the function result cache checks function name"; + + vars: + "res1" data => findprocesses("${init.agent_regex}"); + + classes: + # must not reuse result from previous line + # is reused, produces a type error + "_pass" expression => processexists("${init.agent_regex}"); +} + + +####################################################### + +bundle agent check +{ + methods: + _pass:: + "pass" usebundle => dcs_pass("$(this.promise_filename)"); + + !_pass:: + "pass" usebundle => dcs_fail("$(this.promise_filename)"); +} From 234e8de5289d21a4e0aa95015b209f57c9328bcb Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Thu, 14 Sep 2023 14:56:40 +0200 Subject: [PATCH 2/2] Moved asserts to top of EvalContextFunctionCache functions Two reasons for this: 1. CONTRIBUTING.md says to do this at the top 2. the `ctx` argument was actually used before the assert Ticket: None Changelog: None Signed-off-by: Lars Erik Wik (cherry picked from commit eec977631e9875aa5475aa24d5b276f77ef789a7) --- libpromises/eval_context.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c index c23ae8f07e3..2fdccf6de3c 100644 --- a/libpromises/eval_context.c +++ b/libpromises/eval_context.c @@ -2801,6 +2801,10 @@ bool EvalContextFunctionCacheGet(const EvalContext *ctx, const FnCall *fp ARG_UNUSED, const Rlist *args, Rval *rval_out) { + assert(fp != NULL); + assert(fp->name != NULL); + assert(ctx != NULL); + if (!(ctx->eval_options & EVAL_OPTION_CACHE_SYSTEM_FUNCTIONS)) { return false; @@ -2808,9 +2812,6 @@ bool EvalContextFunctionCacheGet(const EvalContext *ctx, // The cache key is made of the function name and all args values Rlist *args_copy = RlistCopy(args); - assert(fp != NULL); - assert(fp->name != NULL); - assert(ctx != NULL); Rlist *key = RlistPrepend(&args_copy, fp->name, RVAL_TYPE_SCALAR); Rval *rval = FuncCacheMapGet(ctx->function_cache, key); RlistDestroy(key); @@ -2832,6 +2833,10 @@ void EvalContextFunctionCachePut(EvalContext *ctx, const FnCall *fp ARG_UNUSED, const Rlist *args, const Rval *rval) { + assert(fp != NULL); + assert(fp->name != NULL); + assert(ctx != NULL); + if (!(ctx->eval_options & EVAL_OPTION_CACHE_SYSTEM_FUNCTIONS)) { return; @@ -2841,9 +2846,6 @@ void EvalContextFunctionCachePut(EvalContext *ctx, *rval_copy = RvalCopy(*rval); Rlist *args_copy = RlistCopy(args); - assert(fp != NULL); - assert(fp->name != NULL); - assert(ctx != NULL); Rlist *key = RlistPrepend(&args_copy, fp->name, RVAL_TYPE_SCALAR); FuncCacheMapInsert(ctx->function_cache, key, rval_copy);