Project

General

Profile

« Previous | Next » 

Revision 98b56e78

Added by Alexis Mousset about 6 years ago

Fixes #12130: Backport complete variable table performance patch

View differences:

rudder-agent/SOURCES/patches/cfengine/10584-avoid-copy-variable-table.patch
From 55989299c0969500358169a218203f10c584aa06 Mon Sep 17 00:00:00 2001
From: Alexis Mousset <contact@amousset.me>
Date: Fri, 31 Mar 2017 14:06:57 +0200
Subject: [PATCH] CFE-2524: Avoid copying variable table when expanding a
promise
The idea is to stop copying variable tables, which appears to be
one of the most heavy operation the agent does, but to always
lookup the variables in the global table.
The main issue with this change is to manage ambiguities
in the use of the "this" scope. It is used both for special
variables at promise level and as an alias for variables in the
current bundle.
The general idea is to modify VariableResolve to try to resolve
unknown "this"-scoped variables as variables in the current bundle.
The is a second change to force "this"-scopes variables
passed as parameters to a bundle to be qualified explicitly in
the calling bundle.
A non-compatible change could be to stop supporting "this" scope
for non-special bundle, which would allow a clean separation
and avoid the workarounds.
Changelog: Title
---
libpromises/eval_context.c | 65 ++++++++---------
libpromises/expand.c | 3 +-
libpromises/var_expressions.c | 33 +++++++++
libpromises/var_expressions.h | 2 +
libpromises/variable.c | 20 ------
.../acceptance/01_vars/01_basic/this_variables.cf | 83 ++++++++++++++++++++++
.../02_functions/nth_datacontainer.cf.expected | 14 ++--
7 files changed, 157 insertions(+), 63 deletions(-)
create mode 100644 tests/acceptance/01_vars/01_basic/this_variables.cf
diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c
index f967026..6c305b1 100644
--- a/libpromises/eval_context.c
+++ b/libpromises/eval_context.c
@@ -1334,16 +1334,8 @@ void EvalContextStackPushPromiseFrame(EvalContext *ctx, const Promise *owner, bo
EvalContextStackPushFrame(ctx, frame);
- if (copy_bundle_context)
- {
- frame->data.promise.vars = VariableTableCopyLocalized(ctx->global_variables,
- EvalContextStackCurrentBundle(ctx)->ns,
- EvalContextStackCurrentBundle(ctx)->name);
- }
- else
- {
- frame->data.promise.vars = VariableTableNew();
- }
+ // Ignore copy_bundle_context and create an empty table
+ frame->data.promise.vars = VariableTableNew();
if (PromiseGetBundle(owner)->source_path)
{
@@ -1915,6 +1907,8 @@ static VariableTable *GetVariableTableForScope(const EvalContext *ctx,
return frame ? frame->data.body.vars : NULL;
}
+ // "this" variables can be in local or global variable table (when this is used for non-special
+ // varables), so return local as VariableResolve will try global table anyway.
case SPECIAL_SCOPE_THIS:
{
StackFrame *frame = LastStackFrameByType(ctx, STACK_FRAME_TYPE_PROMISE);
@@ -2005,6 +1999,7 @@ static void VarRefStackQualify(const EvalContext *ctx, VarRef *ref)
case STACK_FRAME_TYPE_PROMISE:
case STACK_FRAME_TYPE_PROMISE_ITERATION:
+ // This is necessary to allow special "this" variable to work when used without "this".
VarRefQualify(ref, NULL, SpecialScopeToString(SPECIAL_SCOPE_THIS));
break;
}
@@ -2054,9 +2049,6 @@ bool EvalContextVariablePut(EvalContext *ctx,
* means that an unqualified reference will be looked up in the context of the top
* stack frame. Note that when evaluating a promise, this will qualify a reference
* to the 'this' scope.
- *
- * Ideally, this function should resolve a variable by walking down the stack, but
- * this is pending rework in variable iteration.
*/
static Variable *VariableResolve(const EvalContext *ctx, const VarRef *ref)
{
@@ -2065,16 +2057,29 @@ static Variable *VariableResolve(const EvalContext *ctx, const VarRef *ref)
if (!VarRefIsQualified(ref))
{
VarRef *qref = VarRefCopy(ref);
+ // Warning: This will "qualify" non-scoped variables in a promise to this.variable
VarRefStackQualify(ctx, qref);
Variable *ret = VariableResolve(ctx, qref);
VarRefDestroy(qref);
return ret;
}
+ /*
+ * We will make a first lookup that works in almost all cases: will look for local
+ * or global variables, depending of the current scope.
+ *
+ * The only case it does not work is when we have a "this.custom_variable" variable
+ * because we will search is local promise scope the variable is in the global
+ * variable table. In this case, we will look for the variable in the current bundle.
+ */
+
+ // Get the variable table associated to the scope
VariableTable *table = GetVariableTableForScope(ctx, ref->ns, ref->scope);
+
+ Variable *var;
if (table)
{
- Variable *var = VariableTableGet(table, ref);
+ var = VariableTableGet(table, ref);
if (var)
{
return var;
@@ -2092,6 +2097,17 @@ static Variable *VariableResolve(const EvalContext *ctx, const VarRef *ref)
}
}
+ // Try to qualify "this." variable to "current_bundle."
+ const Bundle *last_bundle = EvalContextStackCurrentBundle(ctx);
+ if (last_bundle && strcmp(ref->scope, SpecialScopeToString(SPECIAL_SCOPE_THIS)) == 0)
+ {
+ VarRef *qref = VarRefCopy(ref);
+ VarRefQualify(qref, last_bundle->ns, last_bundle->name);
+ var = VariableResolve(ctx, qref);
+ VarRefDestroy(qref);
+ return var;
+ }
+
return NULL;
}
@@ -2130,27 +2146,6 @@ const void *EvalContextVariableGet(const EvalContext *ctx, const VarRef *ref, Da
return var->rval.item;
}
}
- else if (!VarRefIsQualified(ref))
- {
- /*
- * FALLBACK: Because VariableResolve currently does not walk the stack
- * (rather, it looks at only the top frame), we do an explicit retry
- * here to qualify an unqualified reference to the current bundle.
- *
- * This is overly complicated, and will go away as soon as
- * VariableResolve can walk the stack (which is pending rework in
- * variable iteration).
- */
- const Bundle *bp = EvalContextStackCurrentBundle(ctx);
- if (bp)
- {
- VarRef *qref = VarRefCopy(ref);
- VarRefQualify(qref, bp->ns, bp->name);
- const void *ret = EvalContextVariableGet(ctx, qref, type_out);
- VarRefDestroy(qref);
- return ret;
- }
- }
if (type_out)
{
diff --git a/libpromises/expand.c b/libpromises/expand.c
index dc90fac..fa8a3a4 100644
--- a/libpromises/expand.c
+++ b/libpromises/expand.c
@@ -256,7 +256,8 @@ PromiseResult ExpandPromise(EvalContext *ctx, const Promise *pp,
* (including body inheritance). */
Promise *pcopy = DeRefCopyPromise(ctx, pp);
- EvalContextStackPushPromiseFrame(ctx, pcopy, true);
+ // TODO: Remove last parameter?
+ EvalContextStackPushPromiseFrame(ctx, pcopy, false);
PromiseIterator *iterctx = PromiseIteratorNew(pcopy);
/* 2. Parse all strings (promiser-promisee-constraints), find all
diff --git a/libpromises/var_expressions.c b/libpromises/var_expressions.c
index afede51..258d58c 100644
--- a/libpromises/var_expressions.c
+++ b/libpromises/var_expressions.c
@@ -320,6 +320,16 @@ VarRef *VarRefParseFromNamespaceAndScope(const char *qualified_name,
{
_ns = NULL;
}
+
+ /*
+ * Force considering non-special "this." variables as unqualified.
+ * This allows qualifying bundle parameters passed as reference with a "this" scope
+ * in the calling bundle.
+ */
+ if (is_this_not_special(scope, lval)) {
+ free(scope);
+ scope = NULL;
+ }
}
VarRef *ref = xmalloc(sizeof(VarRef));
@@ -333,6 +343,29 @@ VarRef *VarRefParseFromNamespaceAndScope(const char *qualified_name,
return ref;
}
+/*
+ * This function will return true if the given variable is
+ * a this.something variable that is an alias to a non-special local variable.
+ */
+bool is_this_not_special(const char *scope, const char *lval) {
+ // TODO: better way to get this list?
+ const char *special_this_variables[] = {"v","k","this","service_policy","promiser","promiser_uid","promiser_gid","promiser_pid","promiser_ppid","bundle","handle","namespace","promise_filename","promise_dirname","promise_linenumber", NULL};
+
+ if (!scope) {
+ return false;
+ }
+
+ if (SpecialScopeFromString(scope) != SPECIAL_SCOPE_THIS) {
+ return false;
+ }
+
+ if (IsStrIn(lval, special_this_variables)) {
+ return false;
+ }
+
+ return true;
+}
+
VarRef *VarRefParse(const char *var_ref_string)
{
return VarRefParseFromNamespaceAndScope(var_ref_string, NULL, NULL, CF_NS, '.');
diff --git a/libpromises/var_expressions.h b/libpromises/var_expressions.h
index 3639a8f..28f77bb 100644
--- a/libpromises/var_expressions.h
+++ b/libpromises/var_expressions.h
@@ -57,6 +57,8 @@ VarRef *VarRefCopy(const VarRef *ref);
VarRef *VarRefCopyLocalized(const VarRef *ref);
VarRef *VarRefCopyIndexless(const VarRef *ref);
+bool is_this_not_special(const char *scope, const char *lval);
+
VarRef *VarRefParse(const char *var_ref_string);
VarRef *VarRefParseFromBundle(const char *var_ref_string, const Bundle *bundle);
diff --git a/libpromises/variable.c b/libpromises/variable.c
index bde3994..ebc6e8a 100644
--- a/libpromises/variable.c
+++ b/libpromises/variable.c
@@ -341,23 +341,3 @@ void VariableTableIteratorDestroy(VariableTableIterator *iter)
free(iter);
}
}
-
-VariableTable *VariableTableCopyLocalized(const VariableTable *table, const char *ns, const char *scope)
-{
- VariableTable *localized_copy = VariableTableNew();
-
- VariableTableIterator *iter = VariableTableIteratorNew(table, ns, scope, NULL);
- Variable *foreign_var = NULL;
- while ((foreign_var = VariableTableIteratorNext(iter)))
- {
- /* TODO why is tags NULL here? Shouldn't it be an exact copy of
- * foreign_var->tags? */
- Variable *localized_var = VariableNew(VarRefCopyLocalized(foreign_var->ref),
- RvalCopy(foreign_var->rval), foreign_var->type,
- NULL, foreign_var->promise);
- VarMapInsert(localized_copy->vars, localized_var->ref, localized_var);
- }
- VariableTableIteratorDestroy(iter);
-
- return localized_copy;
-}
diff --git a/tests/acceptance/01_vars/01_basic/this_variables.cf b/tests/acceptance/01_vars/01_basic/this_variables.cf
new file mode 100644
index 0000000..638d840
--- /dev/null
+++ b/tests/acceptance/01_vars/01_basic/this_variables.cf
@@ -0,0 +1,83 @@
+body common control
+{
+ inputs => { "../../default.cf.sub" };
+ bundlesequence => { default("$(this.promise_filename)") };
+ version => "1.0";
+}
+
+bundle agent test
+{
+ vars:
+ "var" slist => { "var in test" };
+ "var_test" slist => { "var_test in test" };
+
+ methods:
+ "any" usebundle => inter;
+}
+
+bundle agent inter
+{
+ vars:
+ "var" slist => { "var in inter" };
+ "k" slist => { "k in inter" };
+
+ methods:
+ "any1" usebundle => final("test1", @(this.var_test));
+ "any2" usebundle => final("test2", @(var_test));
+ "any3" usebundle => final("test3", @(this.var));
+ "any4" usebundle => final("test4", @(var));
+ "any5" usebundle => final("test5", @(this.k));
+ "any6" usebundle => final("test6", @(k));
+ "any7" usebundle => final("test7", @(this.bundle));
+ "any8" usebundle => final("test8", @(bundle));
+}
+
+bundle agent final(name, param)
+{
+ vars:
+ "${name}" string => "${param}";
+}
+
+bundle agent check
+{
+
+ classes:
+
+ "ok1" not => strcmp("var_test in test", "${final.test1}");
+ "ok2" not => strcmp("var_test in test", "${final.test2}");
+ "ok3" expression => strcmp("var in inter", "${final.test3}");
+ "ok4" expression => strcmp("var in inter", "${final.test4}");
+ "ok5" not => strcmp("k in inter", "${final.test5}");
+ "ok6" expression => strcmp("k in inter", "${final.test6}");
+ "ok7" not => strcmp("inter", "${final.test7}");
+ "ok8" not => strcmp("inter", "${final.test8}");
+
+ "ok" expression => "ok1.ok2.ok3.ok4.ok5.ok6.ok7.ok8";
+
+ reports:
+
+ ok::
+ "$(this.promise_filename) Pass";
+
+ !ok::
+ "FAIL";
+
+ !ok1::
+ "variable '${final.test1}' = 'var_test in test'";
+ !ok2::
+ "variable '${final.test2}' = 'var_test in test'";
+ !ok3::
+ "variable '${final.test3}' != 'var in inter'";
+ !ok4::
+ "variable '${final.test4}' != 'var in inter'";
+ !ok5::
+ "variable '${final.test5}' = 'k in inter'";
+ !ok6::
+ "variable '${final.test6}' != 'k in inter'";
+ !ok7::
+ "variable '${final.test7}' = 'inter'";
+ !ok8::
+ "variable '${final.test8}' = 'inter'";
+}
+
+
diff --git a/tests/acceptance/01_vars/02_functions/nth_datacontainer.cf.expected b/tests/acceptance/01_vars/02_functions/nth_datacontainer.cf.expected
index fc702b7..9375b9d 100644
--- a/tests/acceptance/01_vars/02_functions/nth_datacontainer.cf.expected
+++ b/tests/acceptance/01_vars/02_functions/nth_datacontainer.cf.expected
@@ -1,20 +1,20 @@
jsonstring = {"boolean":true,"boolean_2":false,"integer":20130111,"integer_2":987654321,"list":["chris","dituri","was","here"],"null":null,"object":{"a":true,"b":[1,2,3],"c":"cat","d":108},"string":"Figaro. Figaro. Figaro, Figaro, Figaro... Figaro!","string_2":"Othello? Where art thou now?"}
keys:json = boolean
-keys:json = string
+keys:json = boolean_2
keys:json = integer
+keys:json = integer_2
keys:json = list
+keys:json = null
keys:json = object
-keys:json = integer_2
+keys:json = string
keys:json = string_2
-keys:json = boolean_2
-keys:json = null
primitive:json[boolean] = true
-primitive:json[string] = Figaro. Figaro. Figaro, Figaro, Figaro... Figaro!
+primitive:json[boolean_2] = false
primitive:json[integer] = 20130111
primitive:json[integer_2] = 987654321
-primitive:json[string_2] = Othello? Where art thou now?
-primitive:json[boolean_2] = false
primitive:json[null] = null
+primitive:json[string] = Figaro. Figaro. Figaro, Figaro, Figaro... Figaro!
+primitive:json[string_2] = Othello? Where art thou now?
list:json[0] = chris
list:json[1] = dituri
list:json[2] = was
rudder-agent/SOURCES/patches/cfengine/12130-avoid-copy-variable-table.patch
From 248b9f49025cd10546fe5f3bc78f7af83c29df7f Mon Sep 17 00:00:00 2001
From: Dimitrios Apostolou <jimis@northern.tech>
Date: Thu, 15 Feb 2018 15:24:38 +0100
Subject: [PATCH 1/5] whitespace and style
---
cf-agent/files_editline.c | 13 +++++++------
libpromises/eval_context.c | 16 ++++++++++------
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/cf-agent/files_editline.c b/cf-agent/files_editline.c
index 0edb87e72e..01a0c30eb1 100644
--- a/cf-agent/files_editline.c
+++ b/cf-agent/files_editline.c
@@ -679,12 +679,12 @@ static PromiseResult VerifyLineInsertions(EvalContext *ctx, const Promise *pp, E
else if (!SelectRegion(ctx, *start, &begin_ptr, &end_ptr, a, edcontext))
{
cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_INTERRUPTED, pp, a,
- "The promised line insertion '%s' could not select an edit region in '%s'", pp->promiser,
- edcontext->filename);
+ "The promised line insertion '%s' could not select an edit region in '%s'",
+ pp->promiser, edcontext->filename);
result = PromiseResultUpdate(result, PROMISE_RESULT_INTERRUPTED);
return result;
}
-
+
if (!end_ptr && a.region.select_end && !a.region.select_end_match_eof)
{
cfPS(ctx, LOG_LEVEL_VERBOSE, PROMISE_RESULT_INTERRUPTED, pp, a,
@@ -744,15 +744,16 @@ static PromiseResult VerifyLineInsertions(EvalContext *ctx, const Promise *pp, E
/* Level */
/***************************************************************************/
-static int SelectRegion(EvalContext *ctx, Item *start, Item **begin_ptr, Item **end_ptr, Attributes a,
- EditContext *edcontext)
+static int SelectRegion(EvalContext *ctx, Item *start,
+ Item **begin_ptr, Item **end_ptr,
+ Attributes a, EditContext *edcontext)
/*
This should provide pointers to the first and last line of text that include the
delimiters, since we need to include those in case they are being deleted, etc.
It returns true if a match was identified, else false.
-If no such region matches, begin_ptr and end_ptr should point to NULL
+If no such region matches, begin_ptr and end_ptr should point to NULL
*/
{
diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c
index 409640921f..cef034122f 100644
--- a/libpromises/eval_context.c
+++ b/libpromises/eval_context.c
@@ -130,7 +130,7 @@ struct EvalContext_
/* new package promise evaluation context */
PackagePromiseContext *package_promise_context;
-
+
/* select_end_match_eof context*/
bool select_end_match_eof;
@@ -180,7 +180,7 @@ void AddPackageModuleToContext(const EvalContext *ctx, PackageModuleBody *pm)
/* First check if the body is there added from previous pre-evaluation
* iteration. If it is there update it as we can have new expanded variables. */
ssize_t pm_seq_index;
- if ((pm_seq_index = SeqIndexOf(ctx->package_promise_context->package_modules_bodies,
+ if ((pm_seq_index = SeqIndexOf(ctx->package_promise_context->package_modules_bodies,
pm->name, PackageManagerSeqCompare)) != -1)
{
SeqRemove(ctx->package_promise_context->package_modules_bodies, pm_seq_index);
@@ -602,7 +602,7 @@ void EvalContextHeapPersistentSave(EvalContext *ctx, const char *name, unsigned
ClassRef ref = IDRefQualify(ctx, name);
char *key = ClassRefToString(ref.ns, ref.name);
ClassRefDestroy(ref);
-
+
size_t tags_length = strlen(tags) + 1;
size_t new_info_size = sizeof(PersistentClassInfo) + tags_length;
@@ -882,13 +882,13 @@ void FreePackageManager(PackageModuleBody *manager)
static
PackagePromiseContext *PackagePromiseConfigNew()
{
- PackagePromiseContext *package_promise_defaults =
+ PackagePromiseContext *package_promise_defaults =
xmalloc(sizeof(PackagePromiseContext));
package_promise_defaults->control_package_module = NULL;
package_promise_defaults->control_package_inventory = NULL;
package_promise_defaults->package_modules_bodies =
SeqNew(5, FreePackageManager);
-
+
return package_promise_defaults;
}
@@ -2019,11 +2019,14 @@ static void VarRefStackQualify(const EvalContext *ctx, VarRef *ref)
break;
case STACK_FRAME_TYPE_BUNDLE:
- VarRefQualify(ref, last_frame->data.bundle.owner->ns, last_frame->data.bundle.owner->name);
+ VarRefQualify(ref,
+ last_frame->data.bundle.owner->ns,
+ last_frame->data.bundle.owner->name);
break;
case STACK_FRAME_TYPE_PROMISE:
case STACK_FRAME_TYPE_PROMISE_ITERATION:
+ // Allow special "this" variables to work when used without "this"
VarRefQualify(ref, NULL, SpecialScopeToString(SPECIAL_SCOPE_THIS));
break;
}
@@ -2081,6 +2084,7 @@ static Variable *VariableResolve(const EvalContext *ctx, const VarRef *ref)
{
assert(ref->lval);
+ // RECURSION! Try to qualify non-scoped vars in a promise to "this" scope
if (!VarRefIsQualified(ref))
{
VarRef *qref = VarRefCopy(ref);
From cb345d161dc3bb67ff35c24f74749cf3a92b3c0f Mon Sep 17 00:00:00 2001
From: Dimitrios Apostolou <jimis@northern.tech>
Date: Thu, 15 Feb 2018 16:32:30 +0100
Subject: [PATCH 2/5] Log stack type in vartable debug messages.
---
libpromises/eval_context.c | 27 +++++++++++++++++++++++++--
libpromises/eval_context.h | 3 ++-
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c
index cef034122f..bf7156833c 100644
--- a/libpromises/eval_context.c
+++ b/libpromises/eval_context.c
@@ -52,6 +52,15 @@
#include <conversion.h> /* DataTypeIsIterable */
+static const char *STACK_FRAME_TYPE_STR[STACK_FRAME_TYPE_MAX] = {
+ "BUNDLE",
+ "BODY",
+ "PROMISE_TYPE",
+ "PROMISE",
+ "PROMISE_ITERATION"
+};
+
+
/**
Define FuncCacheMap.
Key: an Rlist (which is linked list of Rvals)
@@ -1079,6 +1088,9 @@ Rlist *EvalContextGetPromiseCallerMethods(EvalContext *ctx) {
case STACK_FRAME_TYPE_PROMISE_TYPE:
break;
+
+ default:
+ ProgrammingError("Unhandled stack frame type");
}
}
return callers_promisers;
@@ -1126,6 +1138,9 @@ JsonElement *EvalContextGetPromiseCallers(EvalContext *ctx) {
JsonObjectAppendString(f, "type", "promise_type");
JsonObjectAppendString(f, "promise_type", frame->data.promise_type.owner->name);
break;
+
+ default:
+ ProgrammingError("Unhandled stack frame type");
}
JsonArrayAppendObject(callers, f);
@@ -1244,7 +1259,8 @@ static void EvalContextStackPushFrame(EvalContext *ctx, StackFrame *frame)
assert(!frame->path);
frame->path = EvalContextStackPath(ctx);
- LogDebug(LOG_MOD_EVALCTX, "PUSHED FRAME (type %d)", frame->type);
+ LogDebug(LOG_MOD_EVALCTX, "PUSHED FRAME (type %s)",
+ STACK_FRAME_TYPE_STR[frame->type]);
}
void EvalContextStackPushBundleFrame(EvalContext *ctx, const Bundle *owner, const Rlist *args, bool inherits_previous)
@@ -1465,7 +1481,8 @@ void EvalContextStackPopFrame(EvalContext *ctx)
}
}
- LogDebug(LOG_MOD_EVALCTX, "POPPED FRAME (type %d)", last_frame_type);
+ LogDebug(LOG_MOD_EVALCTX, "POPPED FRAME (type %s)",
+ STACK_FRAME_TYPE_STR[last_frame_type]);
}
bool EvalContextClassRemove(EvalContext *ctx, const char *ns, const char *name)
@@ -1798,6 +1815,9 @@ char *EvalContextStackPath(const EvalContext *ctx)
PromiseIteratorIndex(frame->data.promise_iteration.iter_ctx));
}
break;
+
+ default:
+ ProgrammingError("Unhandled stack frame type");
}
}
@@ -2029,6 +2049,9 @@ static void VarRefStackQualify(const EvalContext *ctx, VarRef *ref)
// Allow special "this" variables to work when used without "this"
VarRefQualify(ref, NULL, SpecialScopeToString(SPECIAL_SCOPE_THIS));
break;
+
+ default:
+ ProgrammingError("Unhandled stack frame type");
}
}
diff --git a/libpromises/eval_context.h b/libpromises/eval_context.h
index 49cfea0f01..adbc11f2d5 100644
--- a/libpromises/eval_context.h
+++ b/libpromises/eval_context.h
@@ -44,7 +44,8 @@ typedef enum
STACK_FRAME_TYPE_BODY,
STACK_FRAME_TYPE_PROMISE_TYPE,
STACK_FRAME_TYPE_PROMISE,
- STACK_FRAME_TYPE_PROMISE_ITERATION
+ STACK_FRAME_TYPE_PROMISE_ITERATION,
+ STACK_FRAME_TYPE_MAX
} StackFrameType;
typedef struct
From 7aef85e0911dae5d4cb3ef92fecf46bb2cad928f Mon Sep 17 00:00:00 2001
From: Dimitrios Apostolou <jimis@northern.tech>
Date: Thu, 15 Feb 2018 19:55:54 +0100
Subject: [PATCH 3/5] NULL scope means unscoped variable.
---
libpromises/scope.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/libpromises/scope.c b/libpromises/scope.c
index 3507d67854..39ebadaab6 100644
--- a/libpromises/scope.c
+++ b/libpromises/scope.c
@@ -67,7 +67,11 @@ const char *SpecialScopeToString(SpecialScope scope)
SpecialScope SpecialScopeFromString(const char *scope)
{
- if (strcmp("const", scope) == 0)
+ if (scope == NULL)
+ {
+ return SPECIAL_SCOPE_NONE;
+ }
+ else if (strcmp("const", scope) == 0)
{
return SPECIAL_SCOPE_CONST;
}
@@ -99,10 +103,10 @@ SpecialScope SpecialScopeFromString(const char *scope)
{
return SPECIAL_SCOPE_BODY;
}
- else
- {
- return SPECIAL_SCOPE_NONE;
- }
+
+ /* All other scopes fall here, for example all bundle names. It means that
+ * scope was not special. */
+ return SPECIAL_SCOPE_NONE;
}
void ScopeAugment(EvalContext *ctx, const Bundle *bp, const Promise *pp, const Rlist *arguments)
From 48d83dd277321a43a7934cd7108f6e2129c9072f Mon Sep 17 00:00:00 2001
From: Alexis Mousset <contact@amousset.me>
Date: Fri, 31 Mar 2017 14:06:57 +0200
Subject: [PATCH 4/5] CFE-2524: Speedup evalution by not copying variables
table when expanding a promise
The idea is to stop copying variable tables, which appears to be
one of the most heavy operation the agent does, but to always
lookup the variables in the global table.
The main issue with this change is to manage ambiguities
in the use of the "this" scope. It is used both for special
variables at promise level and as an alias for variables in the
current bundle.
The general idea is to modify VariableResolve to try to resolve
unknown "this"-scoped variables as variables in the current bundle.
Then is a second change to force "this"-scoped variables
passed as parameters to a bundle to be qualified explicitly in
the calling bundle.
A non-compatible change could be to stop supporting "this" scope
for non-special bundle, which would allow a clean separation
and avoid the workarounds.
Changelog: Title
Co-Authored-By: Dimitrios Apostolou <jimis@northern.tech>
---
libpromises/eval_context.c | 124 ++++++++++++---------
libpromises/expand.c | 3 +-
libpromises/var_expressions.c | 33 ++++++
libpromises/var_expressions.h | 2 +
libpromises/variable.c | 20 ----
.../acceptance/01_vars/01_basic/this_variables.cf | 63 +++++++++++
.../01_vars/01_basic/this_variables.cf.finish | 3 +
.../02_functions/nth_datacontainer.cf.expected | 14 +--
8 files changed, 180 insertions(+), 82 deletions(-)
create mode 100644 tests/acceptance/01_vars/01_basic/this_variables.cf
create mode 100644 tests/acceptance/01_vars/01_basic/this_variables.cf.finish
diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c
index bf7156833c..115bbf5f56 100644
--- a/libpromises/eval_context.c
+++ b/libpromises/eval_context.c
@@ -1350,16 +1350,8 @@ void EvalContextStackPushPromiseFrame(EvalContext *ctx, const Promise *owner, bo
EvalContextStackPushFrame(ctx, frame);
- if (copy_bundle_context)
- {
- frame->data.promise.vars = VariableTableCopyLocalized(ctx->global_variables,
- EvalContextStackCurrentBundle(ctx)->ns,
- EvalContextStackCurrentBundle(ctx)->name);
- }
- else
- {
- frame->data.promise.vars = VariableTableNew();
- }
+ // Ignore copy_bundle_context and create an empty table
+ frame->data.promise.vars = VariableTableNew();
if (PromiseGetBundle(owner)->source_path)
{
@@ -1954,6 +1946,8 @@ static VariableTable *GetVariableTableForScope(const EvalContext *ctx,
return frame ? frame->data.body.vars : NULL;
}
+ // "this" variables can be in local or global variable table (when this is used for non-special
+ // varables), so return local as VariableResolve will try global table anyway.
case SPECIAL_SCOPE_THIS:
{
StackFrame *frame = LastStackFrameByType(ctx, STACK_FRAME_TYPE_PROMISE);
@@ -2094,33 +2088,15 @@ bool EvalContextVariablePut(EvalContext *ctx,
return true;
}
-/*
- * Looks up a variable in the the context of the 'current scope'. This basically
- * means that an unqualified reference will be looked up in the context of the top
- * stack frame. Note that when evaluating a promise, this will qualify a reference
- * to the 'this' scope.
- *
- * Ideally, this function should resolve a variable by walking down the stack, but
- * this is pending rework in variable iteration.
- */
-static Variable *VariableResolve(const EvalContext *ctx, const VarRef *ref)
+static Variable *VariableResolve2(const EvalContext *ctx, const VarRef *ref)
{
- assert(ref->lval);
-
- // RECURSION! Try to qualify non-scoped vars in a promise to "this" scope
- if (!VarRefIsQualified(ref))
- {
- VarRef *qref = VarRefCopy(ref);
- VarRefStackQualify(ctx, qref);
- Variable *ret = VariableResolve(ctx, qref);
- VarRefDestroy(qref);
- return ret;
- }
-
+ // Get the variable table associated to the scope
VariableTable *table = GetVariableTableForScope(ctx, ref->ns, ref->scope);
+
+ Variable *var;
if (table)
{
- Variable *var = VariableTableGet(table, ref);
+ var = VariableTableGet(table, ref);
if (var)
{
return var;
@@ -2141,6 +2117,67 @@ static Variable *VariableResolve(const EvalContext *ctx, const VarRef *ref)
return NULL;
}
+/*
+ * Looks up a variable in the the context of the 'current scope'. This
+ * basically means that an unqualified reference will be looked up in the
+ * context of the top stack frame.
+ *
+ * Note that when evaluating a promise, this
+ * will qualify a reference to 'this' scope and when evaluating a body, it
+ * will qualify a reference to 'body' scope.
+ */
+static Variable *VariableResolve(const EvalContext *ctx, const VarRef *ref)
+{
+ assert(ref->lval);
+
+ /* We will make a first lookup that works in almost all cases: will look
+ * for local or global variables, depending of the current scope. */
+
+ Variable *var1 = VariableResolve2(ctx, ref);
+ if (var1)
+ {
+ return var1;
+ }
+
+ /* Try to qualify non-scoped vars to the scope:
+ "this" for promises, "body" for bodies, current bundle for bundles. */
+ VarRef *scoped_ref = NULL;
+ if (!VarRefIsQualified(ref))
+ {
+ scoped_ref = VarRefCopy(ref);
+ VarRefStackQualify(ctx, scoped_ref);
+ Variable *var2 = VariableResolve2(ctx, scoped_ref);
+ if (var2)
+ {
+ VarRefDestroy(scoped_ref);
+ return var2;
+ }
+ ref = scoped_ref; /* continue with the scoped variable */
+ }
+
+ const Bundle *last_bundle = EvalContextStackCurrentBundle(ctx);
+
+ /* If we are in a promise or a body, the variable might be coming from the
+ * last bundle. So try a last lookup with "this" or "body" special scopes
+ * replaced with the last bundle. */
+
+ if ((SpecialScopeFromString(ref->scope) == SPECIAL_SCOPE_THIS ||
+ SpecialScopeFromString(ref->scope) == SPECIAL_SCOPE_BODY)
+ && last_bundle != NULL)
+ {
+ VarRef *ref2 = VarRefCopy(ref);
+ VarRefQualify(ref2, last_bundle->ns, last_bundle->name);
+ Variable *var3 = VariableResolve2(ctx, ref2);
+
+ VarRefDestroy(scoped_ref);
+ VarRefDestroy(ref2);
+ return var3;
+ }
+
+ VarRefDestroy(scoped_ref);
+ return NULL;
+}
+
/**
*
* @NOTE NULL is a valid return value if #type_out is of list type and the
@@ -2176,27 +2213,6 @@ const void *EvalContextVariableGet(const EvalContext *ctx, const VarRef *ref, Da
return var->rval.item;
}
}
- else if (!VarRefIsQualified(ref))
- {
- /*
- * FALLBACK: Because VariableResolve currently does not walk the stack
- * (rather, it looks at only the top frame), we do an explicit retry
- * here to qualify an unqualified reference to the current bundle.
- *
- * This is overly complicated, and will go away as soon as
- * VariableResolve can walk the stack (which is pending rework in
- * variable iteration).
- */
- const Bundle *bp = EvalContextStackCurrentBundle(ctx);
- if (bp)
- {
- VarRef *qref = VarRefCopy(ref);
- VarRefQualify(qref, bp->ns, bp->name);
- const void *ret = EvalContextVariableGet(ctx, qref, type_out);
- VarRefDestroy(qref);
- return ret;
- }
- }
if (type_out)
{
diff --git a/libpromises/expand.c b/libpromises/expand.c
index 91426420eb..eaa7d0de8d 100644
--- a/libpromises/expand.c
+++ b/libpromises/expand.c
@@ -256,7 +256,8 @@ PromiseResult ExpandPromise(EvalContext *ctx, const Promise *pp,
* (including body inheritance). */
Promise *pcopy = DeRefCopyPromise(ctx, pp);
- EvalContextStackPushPromiseFrame(ctx, pcopy, true);
+ // TODO: Remove last parameter?
+ EvalContextStackPushPromiseFrame(ctx, pcopy, false);
PromiseIterator *iterctx = PromiseIteratorNew(pcopy);
/* 2. Parse all strings (promiser-promisee-constraints), find all
diff --git a/libpromises/var_expressions.c b/libpromises/var_expressions.c
index 6250e3b067..94d478cffb 100644
--- a/libpromises/var_expressions.c
+++ b/libpromises/var_expressions.c
@@ -317,6 +317,16 @@ VarRef *VarRefParseFromNamespaceAndScope(const char *qualified_name,
{
_ns = NULL;
}
+
+ /*
+ * Force considering non-special "this." variables as unqualified.
+ * This allows qualifying bundle parameters passed as reference with a "this" scope
+ * in the calling bundle.
+ */
+ if (is_this_not_special(scope, lval)) {
+ free(scope);
+ scope = NULL;
+ }
}
VarRef *ref = xmalloc(sizeof(VarRef));
@@ -330,6 +340,29 @@ VarRef *VarRefParseFromNamespaceAndScope(const char *qualified_name,
return ref;
}
+/*
+ * This function will return true if the given variable is
+ * a this.something variable that is an alias to a non-special local variable.
+ */
+bool is_this_not_special(const char *scope, const char *lval) {
+ // TODO: better way to get this list?
+ const char *special_this_variables[] = {"v","k","this","service_policy","promiser","promiser_uid","promiser_gid","promiser_pid","promiser_ppid","bundle","handle","namespace","promise_filename","promise_dirname","promise_linenumber", NULL};
+
+ if (!scope) {
+ return false;
+ }
+
+ if (SpecialScopeFromString(scope) != SPECIAL_SCOPE_THIS) {
+ return false;
+ }
+
+ if (IsStrIn(lval, special_this_variables)) {
+ return false;
+ }
+
+ return true;
+}
+
VarRef *VarRefParse(const char *var_ref_string)
{
return VarRefParseFromNamespaceAndScope(var_ref_string, NULL, NULL, CF_NS, '.');
diff --git a/libpromises/var_expressions.h b/libpromises/var_expressions.h
index 716b4faa0f..92f5b89f4d 100644
--- a/libpromises/var_expressions.h
+++ b/libpromises/var_expressions.h
@@ -57,6 +57,8 @@ VarRef *VarRefCopy(const VarRef *ref);
VarRef *VarRefCopyLocalized(const VarRef *ref);
VarRef *VarRefCopyIndexless(const VarRef *ref);
+bool is_this_not_special(const char *scope, const char *lval);
+
VarRef *VarRefParse(const char *var_ref_string);
VarRef *VarRefParseFromBundle(const char *var_ref_string, const Bundle *bundle);
diff --git a/libpromises/variable.c b/libpromises/variable.c
index deabd5c933..ccc13a9c96 100644
--- a/libpromises/variable.c
+++ b/libpromises/variable.c
@@ -341,23 +341,3 @@ void VariableTableIteratorDestroy(VariableTableIterator *iter)
free(iter);
}
}
-
-VariableTable *VariableTableCopyLocalized(const VariableTable *table, const char *ns, const char *scope)
-{
- VariableTable *localized_copy = VariableTableNew();
-
- VariableTableIterator *iter = VariableTableIteratorNew(table, ns, scope, NULL);
- Variable *foreign_var = NULL;
- while ((foreign_var = VariableTableIteratorNext(iter)))
- {
- /* TODO why is tags NULL here? Shouldn't it be an exact copy of
- * foreign_var->tags? */
- Variable *localized_var = VariableNew(VarRefCopyLocalized(foreign_var->ref),
- RvalCopy(foreign_var->rval), foreign_var->type,
- NULL, foreign_var->promise);
- VarMapInsert(localized_copy->vars, localized_var->ref, localized_var);
- }
- VariableTableIteratorDestroy(iter);
-
- return localized_copy;
-}
diff --git a/tests/acceptance/01_vars/01_basic/this_variables.cf b/tests/acceptance/01_vars/01_basic/this_variables.cf
new file mode 100644
index 0000000000..560573c3bd
--- /dev/null
+++ b/tests/acceptance/01_vars/01_basic/this_variables.cf
@@ -0,0 +1,63 @@
+# This was created because of a bug while working on purging variable table
+# copying for CFE-2524. The issue was that variables were not found when
+# looked up deep inside the INI_SECTION body.
+
+body common control
+{
+ inputs => { "../../default.cf.sub" };
+ bundlesequence => { default("$(this.promise_filename)") };
+ version => "1.0";
+}
+
+bundle agent init
+{
+ files:
+ "$(G.testfile).expected"
+ copy_from => local_cp("$(this.promise_filename).finish");
+}
+
+bundle agent test
+{
+ vars:
+ "var" slist => { "var in test" };
+ "var_test" slist => { "var_test in test" };
+
+ files:
+ "$(G.testfile).actual"
+ create => "true",
+ edit_line => test_edit_line("testline");
+}
+
+bundle edit_line test_edit_line(line)
+{
+
+ vars:
+ "sectionName" string => "test";
+
+ insert_lines:
+ "[$(sectionName)]
+[end]"
+ location => start;
+
+ "${line}"
+ select_region => INI_section(escape("$(sectionName)"));
+
+}
+
+bundle agent check
+{
+ methods:
+ "check"
+ usebundle => dcs_if_diff( "$(G.testfile).actual",
+ "$(G.testfile).expected",
+ "pass", "_fail");
+
+ # Fail the test if any of the files fail.
+ "fail"
+ usebundle => dcs_fail( $(this.promise_filename) ),
+ ifvarclass => "_fail";
+
+ pass::
+ "pass"
+ usebundle => dcs_pass( $(this.promise_filename) );
+}
diff --git a/tests/acceptance/01_vars/01_basic/this_variables.cf.finish b/tests/acceptance/01_vars/01_basic/this_variables.cf.finish
new file mode 100644
index 0000000000..904bb6cc72
--- /dev/null
+++ b/tests/acceptance/01_vars/01_basic/this_variables.cf.finish
@@ -0,0 +1,3 @@
+[test]
+testline
+[end]
diff --git a/tests/acceptance/01_vars/02_functions/nth_datacontainer.cf.expected b/tests/acceptance/01_vars/02_functions/nth_datacontainer.cf.expected
index fc702b7038..9375b9d119 100644
--- a/tests/acceptance/01_vars/02_functions/nth_datacontainer.cf.expected
+++ b/tests/acceptance/01_vars/02_functions/nth_datacontainer.cf.expected
@@ -1,20 +1,20 @@
jsonstring = {"boolean":true,"boolean_2":false,"integer":20130111,"integer_2":987654321,"list":["chris","dituri","was","here"],"null":null,"object":{"a":true,"b":[1,2,3],"c":"cat","d":108},"string":"Figaro. Figaro. Figaro, Figaro, Figaro... Figaro!","string_2":"Othello? Where art thou now?"}
keys:json = boolean
-keys:json = string
+keys:json = boolean_2
keys:json = integer
+keys:json = integer_2
keys:json = list
+keys:json = null
keys:json = object
-keys:json = integer_2
+keys:json = string
keys:json = string_2
-keys:json = boolean_2
-keys:json = null
primitive:json[boolean] = true
-primitive:json[string] = Figaro. Figaro. Figaro, Figaro, Figaro... Figaro!
+primitive:json[boolean_2] = false
primitive:json[integer] = 20130111
primitive:json[integer_2] = 987654321
-primitive:json[string_2] = Othello? Where art thou now?
-primitive:json[boolean_2] = false
primitive:json[null] = null
+primitive:json[string] = Figaro. Figaro. Figaro, Figaro, Figaro... Figaro!
+primitive:json[string_2] = Othello? Where art thou now?
list:json[0] = chris
list:json[1] = dituri
list:json[2] = was
From e4d90658f3ff71d4593c00da970655dd7b2c3552 Mon Sep 17 00:00:00 2001
From: Dimitrios Apostolou <jimis@northern.tech>
Date: Thu, 15 Feb 2018 20:29:42 +0100
Subject: [PATCH 5/5] Purge unused variable.
---
cf-agent/verify_packages.c | 12 ++++++------
libpromises/eval_context.c | 4 ++--
libpromises/eval_context.h | 2 +-
libpromises/expand.c | 3 +--
4 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/cf-agent/verify_packages.c b/cf-agent/verify_packages.c
index e8644f931c..54b8564bca 100644
--- a/cf-agent/verify_packages.c
+++ b/cf-agent/verify_packages.c
@@ -2639,7 +2639,7 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
Log(LOG_LEVEL_VERBOSE, "Command does not allow arguments");
PromiseResult result = PROMISE_RESULT_NOOP;
- EvalContextStackPushPromiseFrame(ctx, pp, false);
+ EvalContextStackPushPromiseFrame(ctx, pp);
if (EvalContextStackPushPromiseIterationFrame(ctx, NULL))
{
if (ExecPackageCommand(ctx, command_string, verify, true, a, pp, &result))
@@ -2692,7 +2692,7 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
}
PromiseResult result = PROMISE_RESULT_NOOP;
- EvalContextStackPushPromiseFrame(ctx, ppi, false);
+ EvalContextStackPushPromiseFrame(ctx, ppi);
if (EvalContextStackPushPromiseIterationFrame(ctx, NULL))
{
if (ExecPackageCommand(ctx, command_string, verify, true, a, ppi, &result))
@@ -2753,7 +2753,7 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
}
PromiseResult result = PROMISE_RESULT_NOOP;
- EvalContextStackPushPromiseFrame(ctx, pp, false);
+ EvalContextStackPushPromiseFrame(ctx, pp);
if (EvalContextStackPushPromiseIterationFrame(ctx, NULL))
{
bool ok = ExecPackageCommand(ctx, command_string, verify, true, a, pp, &result);
@@ -2910,7 +2910,7 @@ static bool ExecutePatch(EvalContext *ctx, const PackageManager *schedule, Packa
Log(LOG_LEVEL_VERBOSE, "Command does not allow arguments");
PromiseResult result = PROMISE_RESULT_NOOP;
- EvalContextStackPushPromiseFrame(ctx, pp, false);
+ EvalContextStackPushPromiseFrame(ctx, pp);
if (EvalContextStackPushPromiseIterationFrame(ctx, NULL))
{
if (ExecPackageCommand(ctx, command_string, false, true, a, pp, &result))
@@ -2943,7 +2943,7 @@ static bool ExecutePatch(EvalContext *ctx, const PackageManager *schedule, Packa
strcat(offset, pi->name);
PromiseResult result = PROMISE_RESULT_NOOP;
- EvalContextStackPushPromiseFrame(ctx, pp, false);
+ EvalContextStackPushPromiseFrame(ctx, pp);
if (EvalContextStackPushPromiseIterationFrame(ctx, NULL))
{
if (ExecPackageCommand(ctx, command_string, false, true, a, pp, &result))
@@ -2982,7 +2982,7 @@ static bool ExecutePatch(EvalContext *ctx, const PackageManager *schedule, Packa
}
PromiseResult result = PROMISE_RESULT_NOOP;
- EvalContextStackPushPromiseFrame(ctx, pp, false);
+ EvalContextStackPushPromiseFrame(ctx, pp);
if (EvalContextStackPushPromiseIterationFrame(ctx, NULL))
{
bool ok = ExecPackageCommand(ctx, command_string, false, true, a, pp, &result);
diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c
index 115bbf5f56..e97fb22026 100644
--- a/libpromises/eval_context.c
+++ b/libpromises/eval_context.c
@@ -1339,7 +1339,7 @@ void EvalContextStackPushPromiseTypeFrame(EvalContext *ctx, const PromiseType *o
EvalContextStackPushFrame(ctx, frame);
}
-void EvalContextStackPushPromiseFrame(EvalContext *ctx, const Promise *owner, bool copy_bundle_context)
+void EvalContextStackPushPromiseFrame(EvalContext *ctx, const Promise *owner)
{
assert(LastStackFrame(ctx, 0));
assert(LastStackFrame(ctx, 0)->type == STACK_FRAME_TYPE_PROMISE_TYPE);
@@ -1350,7 +1350,7 @@ void EvalContextStackPushPromiseFrame(EvalContext *ctx, const Promise *owner, bo
EvalContextStackPushFrame(ctx, frame);
- // Ignore copy_bundle_context and create an empty table
+ // create an empty table
frame->data.promise.vars = VariableTableNew();
if (PromiseGetBundle(owner)->source_path)
diff --git a/libpromises/eval_context.h b/libpromises/eval_context.h
index adbc11f2d5..a87618d3ce 100644
--- a/libpromises/eval_context.h
+++ b/libpromises/eval_context.h
@@ -141,7 +141,7 @@ Rlist *EvalContextGetPromiseCallerMethods(EvalContext *ctx);
void EvalContextStackPushBundleFrame(EvalContext *ctx, const Bundle *owner, const Rlist *args, bool inherits_previous);
void EvalContextStackPushBodyFrame(EvalContext *ctx, const Promise *caller, const Body *body, const Rlist *args);
void EvalContextStackPushPromiseTypeFrame(EvalContext *ctx, const PromiseType *owner);
-void EvalContextStackPushPromiseFrame(EvalContext *ctx, const Promise *owner, bool copy_bundle_context);
+void EvalContextStackPushPromiseFrame(EvalContext *ctx, const Promise *owner);
Promise *EvalContextStackPushPromiseIterationFrame(EvalContext *ctx, const PromiseIterator *iter_ctx);
void EvalContextStackPopFrame(EvalContext *ctx);
const char *EvalContextStackToString(EvalContext *ctx);
diff --git a/libpromises/expand.c b/libpromises/expand.c
index eaa7d0de8d..e334ffef94 100644
--- a/libpromises/expand.c
+++ b/libpromises/expand.c
@@ -256,8 +256,7 @@ PromiseResult ExpandPromise(EvalContext *ctx, const Promise *pp,
* (including body inheritance). */
Promise *pcopy = DeRefCopyPromise(ctx, pp);
- // TODO: Remove last parameter?
- EvalContextStackPushPromiseFrame(ctx, pcopy, false);
+ EvalContextStackPushPromiseFrame(ctx, pcopy);
PromiseIterator *iterctx = PromiseIteratorNew(pcopy);
/* 2. Parse all strings (promiser-promisee-constraints), find all

Also available in: Unified diff