CFE-2788: Added main bundles with special semantics #3118

olehermanse · 2018-05-11T15:09:49Z

Example usage:

root@dev core $ cat imported.cf
bundle agent __main__
{
        reports:
                "Hello __main__, imported.cf can be executed directly";
}

bundle agent useful_functionality(caller)
{
        reports:
                "$(this.promise_filename) is used as a library from $(caller)";
}
root@dev core $ cat entry.cf
body common control
{
        inputs => {"${sys.policy_entry_dirname}/imported.cf"};
}

bundle agent main
{
        methods:
                "Library call"
                         usebundle => useful_functionality("$(sys.policy_entry_basename)");
        reports:
                "Hello main";
}
root@dev core $ sudo /var/cfengine/bin/cf-agent -f ./entry.cf -K
R: /home/vagrant/cfe/core/imported.cf is used as a library from entry.cf
R: Hello main
root@dev core $ sudo /var/cfengine/bin/cf-agent -f ./imported.cf -K
R: Hello __main__, imported.cf can be executed directly
root@dev core $

codecov · 2018-05-11T15:27:26Z

Codecov Report

Merging #3118 into master will decrease coverage by 0.01%.
The diff coverage is 57.89%.

@@            Coverage Diff             @@
##           master    #3118      +/-   ##
==========================================
- Coverage   61.96%   61.95%   -0.02%     
==========================================
  Files         210      210              
  Lines       45921    45978      +57     
==========================================
+ Hits        28455    28485      +30     
- Misses      17466    17493      +27

Impacted Files	Coverage Δ
libutils/sequence.h	`100% <ø> (ø)`	⬆️
libutils/sequence.c	`83.33% <0%> (-5.43%)`	⬇️
libpromises/generic_agent.c	`76.5% <100%> (+0.02%)`	⬆️
libpromises/eval_context.c	`86.63% <100%> (+0.08%)`	⬆️
libpromises/loading.c	`80.53% <61.53%> (-2.1%)`	⬇️
libpromises/files_names.c	`73.02% <72.72%> (-0.02%)`	⬇️
libpromises/locks.c	`81.29% <0%> (-0.75%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8d14a7...4777725. Read the comment docs.

vpodzime

Overall I think this is a great idea. We just need to be careful. As always. :)

vpodzime · 2018-05-14T09:26:01Z

libutils/sequence.c

+        }
+        s->data[to] = s->data[from];
+        ++from;
+        ++to;


I'd prefer else here instead of continue, but YMMV.

I (obviously) preferred the continue, but maybe I'll change it. Just because it's you ;)

vpodzime · 2018-05-14T09:33:58Z

libpromises/loading.c

@@ -253,6 +255,44 @@ static void ShowContext(EvalContext *ctx)
    SeqDestroy(soft_contexts);
 }

+static void RenameMainBundle(EvalContext *ctx, GenericAgentConfig *config, Policy *policy)


config seems to be unused here.

vpodzime · 2018-05-14T09:36:24Z

libpromises/loading.c

+        {
+            const char *const path = bundle->source_path;
+            char *abspath = GetAbsolutePath(path);
+            if (StringSafeEqual(abspath, entry_point))


Do we have to worry about symlinks here?

Yes, I thought about this too, I'll see if we have a realpath function.

vpodzime · 2018-05-14T09:37:28Z

libpromises/loading.c

+            {
+                Log(LOG_LEVEL_VERBOSE, "Redefining __main__ bundle from file %s to be main", abspath);
+                free(bundle->name);
+                name = bundle->name = xstrdup("main");


Ugh. Why? This is ugly and there's no need to assign to name here, right?

You'll have to be more specific, what are you saying "Why?" to?

Assigning to name isn't strictly necessary.

Do you dislike the renaming to "main"? I preferred this over other more complicated solutions. It seems to be pretty backwards compatible, and is easy to explain the behavior.

I meant the double-assignment, I only consider it okay when initializing variables. The name main is fine with me.

Ok, in that case, fixed! :)

vpodzime · 2018-05-14T09:38:15Z

libpromises/loading.c

+                Log(LOG_LEVEL_VERBOSE, "Redefining __main__ bundle from file %s to be main", abspath);
+                free(bundle->name);
+                name = bundle->name = xstrdup("main");
+                main_counter += 1;


Shouldn't this be a global variable then?

What should be a global variable?

Well, there should only be one main bundle across all the policy files loaded, right? Or is it okay if there are multiple ones in different files?

Right, makes sense. I removed it though, because we should already have checks for this.

vpodzime · 2018-05-14T09:39:24Z

libpromises/loading.c

+            {
+                Log(LOG_LEVEL_VERBOSE, "Dropping __main__ bundle from file %s (entry point: %s)", abspath, entry_point);
+                removed = true;
+                SeqSet(bundles, i, NULL);


Shouldn't the bundle be properly destroyed (deallocated) here?

SeqSet calls destroy function, so no, that would be a double free.

Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

If the bundle is defined in the entry policy it will be defined as main. If the bundle is defined elsewhere, it will be removed. This makes it easy to make importable library policy which can also be executed directly. Changelog: Commit Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

olehermanse · 2018-05-15T18:25:41Z

@vpodzime I addressed all your comments, added a realpath function for symlinks and got rid of the unnecessary strdup since the name buffer can be reused. Please re-review.

vpodzime · 2018-05-16T09:18:51Z

vpodzime

Looks good to me now, thanks! Let's see if the concerns about realpath() from its man page are just rumors.

vpodzime · 2018-05-16T09:26:39Z

libpromises/loading.c

+                Log(LOG_LEVEL_VERBOSE,
+                    "Redefining __main__ bundle from file %s to be main",
+                    abspath);
+                strncpy(bundle->name, "main", 4+1);


Too bad one cannot write ++4 😉

olehermanse · 2018-05-16T23:34:16Z

Did some more manual testing. Seems to work as expected:

root@dev vagrant $ cat a.cf
body common control
{
        inputs => {"${sys.policy_entry_dirname}/b.cf", "${sys.policy_entry_dirname}/c.cf"};
}

bundle agent __main__
{
        methods:
                "a" usebundle => greet_a("$(sys.policy_entry_basename)");
                "b" usebundle => greet_b("$(sys.policy_entry_basename)");

        reports:
                "Hello __main__, from a.cf";
}

bundle agent greet_a(message)
{
        reports:
                "Hello, $(this.promise_filename) - message: $(message)";
}root@dev vagrant $ cat b.cf
bundle agent __main__
{
        reports:
                "Hello __main__, from b.cf";
}

bundle agent greet_b(message)
{
        methods:
                "c" usebundle => greet_c("$(sys.policy_entry_basename)");
        reports:
                "Hello, $(this.promise_filename) - message: $(message)";
}root@dev vagrant $ cat c.cf
bundle agent __main__
{
        reports:
                "Hello __main__, from c.cf";
}

bundle agent __main__
{
        reports:
                "This should error if c.cf is run directly";
}

bundle agent greet_c(message)
{
        reports:
                "Hello, $(this.promise_filename) - message: $(message)";
root@dev vagrant $ /var/cfengine/bin/cf-agent -K -f ./a.cf
R: Hello, /home/vagrant/./a.cf - message: a.cf
R: Hello, /home/vagrant/c.cf - message: a.cf
R: Hello, /home/vagrant/b.cf - message: a.cf
R: Hello __main__, from a.cf
root@dev vagrant $ /var/cfengine/bin/cf-agent -K -f ./c.cf
./c.cf:1:0: error: Duplicate definition of bundle main with type agent
./c.cf:7:0: error: Duplicate definition of bundle main with type agent
   error: There are syntax errors in policy files
   error: Policy failed validation with command '"/var/cfengine/bin/cf-promises" -c "./c.cf"'
   error: Failsafe condition triggered. Interactive session detected, skipping failsafe.cf execution.
   error: Error reading CFEngine policy. Exiting...
root@dev vagrant $

olehermanse · 2018-05-16T23:41:50Z

The failures are unrelated. Merging.

olehermanse · 2018-05-16T23:42:48Z

Added follow-up tasks:
https://tracker.mender.io/browse/CFE-2820
https://tracker.mender.io/browse/CFE-2819

olehermanse · 2018-05-23T16:24:14Z

This has been cherry-picked in #3137

olehermanse requested a review from vpodzime May 11, 2018 15:10

olehermanse force-pushed the __main__ branch from 9ee60fb to b94849a Compare May 11, 2018 15:27

olehermanse changed the title ~~Added __main__ bundles with special semantics~~ CFE-2788: Added __main__ bundles with special semantics May 11, 2018

olehermanse force-pushed the __main__ branch from b94849a to e7c99ae Compare May 11, 2018 15:33

vpodzime reviewed May 14, 2018

View reviewed changes

olehermanse force-pushed the __main__ branch 2 times, most recently from 1aa7067 to 7324cf3 Compare May 14, 2018 18:31

olehermanse added 3 commits May 15, 2018 14:52

Added function to remove NULL from sequence

5fd9c86

Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

Added policy entry point to eval context

ffcf6ba

Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

Added GetRealPath function

0d870d1

Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

olehermanse force-pushed the __main__ branch from 7324cf3 to f39207d Compare May 15, 2018 12:52

olehermanse force-pushed the __main__ branch from f39207d to 4777725 Compare May 15, 2018 18:22

vpodzime approved these changes May 16, 2018

View reviewed changes

olehermanse merged commit 8ee5d91 into cfengine:master May 16, 2018

olehermanse deleted the __main__ branch May 16, 2018 23:41

olehermanse added the cherry-pick? Fixes which may need to be cherry-picked to LTS branches label May 16, 2018

olehermanse mentioned this pull request May 23, 2018

Added special behavior for bundles named __main__ (3.12.x) #3137

Merged

olehermanse removed the cherry-pick? Fixes which may need to be cherry-picked to LTS branches label May 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CFE-2788: Added main bundles with special semantics #3118

CFE-2788: Added main bundles with special semantics #3118

olehermanse commented May 11, 2018

codecov bot commented May 11, 2018 •

edited

vpodzime left a comment

vpodzime May 14, 2018

olehermanse May 14, 2018

vpodzime May 14, 2018

olehermanse May 14, 2018

vpodzime May 14, 2018

olehermanse May 14, 2018

vpodzime May 14, 2018

olehermanse May 14, 2018

vpodzime May 15, 2018

olehermanse May 15, 2018

vpodzime May 14, 2018

olehermanse May 14, 2018

vpodzime May 15, 2018

olehermanse May 15, 2018

vpodzime May 14, 2018

olehermanse May 14, 2018

olehermanse commented May 15, 2018

vpodzime commented May 16, 2018

vpodzime left a comment

vpodzime May 16, 2018

olehermanse commented May 16, 2018

olehermanse commented May 16, 2018

olehermanse commented May 16, 2018

olehermanse commented May 23, 2018

CFE-2788: Added __main__ bundles with special semantics #3118

CFE-2788: Added __main__ bundles with special semantics #3118

Conversation

olehermanse commented May 11, 2018

codecov bot commented May 11, 2018 • edited

Codecov Report

vpodzime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olehermanse commented May 15, 2018

vpodzime commented May 16, 2018

vpodzime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olehermanse commented May 16, 2018

olehermanse commented May 16, 2018

olehermanse commented May 16, 2018

olehermanse commented May 23, 2018

CFE-2788: Added main bundles with special semantics #3118

CFE-2788: Added main bundles with special semantics #3118

codecov bot commented May 11, 2018 •

edited