Suspend core by default when calling plugin command functions

docs/changelog.txt

@@ -70,6 +70,9 @@ Template for new versions:
 
 ## Removed
 
+## Internals
+- Plugin command callbacks are now called with the core suspended by default so DF memory is always safe to access
+
 # 50.15-r1.2
 
 ## Misc Improvements

library/PluginManager.cpp

@@ -472,41 +472,32 @@ command_result Plugin::invoke(color_ostream &out, const std::string & command, s
     Core & c = Core::getInstance();
     command_result cr = CR_NOT_IMPLEMENTED;
     access->lock_add();
-    if(state == PS_LOADED)
-    {
-        for (size_t i = 0; i < commands.size();i++)
-        {
-            PluginCommand &cmd = commands[i];
-            if(cmd.name == command)
-            {
-                // running interactive things from some other source than the console would break it
-                if(!out.is_console() && cmd.interactive)
-                    cr = CR_NEEDS_CONSOLE;
-                else if (cmd.guard)
-                {
-                    // Execute hotkey commands in a way where they can
-                    // expect their guard conditions to be matched,
-                    // so as to avoid duplicating checks.
-                    // This means suspending the core beforehand.
-                    CoreSuspender suspend(&c);
-                    df::viewscreen *top = c.getTopViewscreen();
-
-                    if (!cmd.guard(top))
-                    {
-                        out.printerr("Could not invoke %s: unsuitable UI state.\n", command.c_str());
-                        cr = CR_WRONG_USAGE;
-                    }
-                    else
-                    {
-                        cr = cmd.function(out, parameters);
-                    }
+    if (state == PS_LOADED) {
+        for (auto & cmd : commands) {
+            if (cmd.name != command)
+                continue;
+
+            // running interactive things from some other source than the console would break it
+            if (!out.is_console() && cmd.interactive)
+                cr = CR_NEEDS_CONSOLE;
+            else if (cmd.guard) {
+                CoreSuspender suspend(&c);
+                if (!cmd.guard(c.getTopViewscreen())) {
+                    out.printerr("Could not invoke %s: unsuitable UI state.\n", command.c_str());
+                    cr = CR_WRONG_USAGE;
                 }
-                else
-                {
+                else {
                     cr = cmd.function(out, parameters);
                 }
-                break;
             }
+            else if (cmd.unlocked) {
+                cr = cmd.function(out, parameters);
+            }
+            else {
+                CoreSuspender suspend(&c);
+                cr = cmd.function(out, parameters);
+            }
+            break;
         }
     }
     access->lock_sub();

library/include/PluginManager.h

@@ -99,11 +99,12 @@ namespace DFHack
                       const char * _description,
                       command_function function_,
                       bool interactive_ = false,
+                      bool unlocked_ = false,
                       const char * usage_ = ""
                      )
-            : name(_name), description(_description),
-              function(function_), interactive(interactive_),
-              guard(NULL), usage(usage_)
+            : name(_name), description(_description), function(function_),
+              interactive(interactive_), unlocked(unlocked_), guard(NULL),
+              usage(usage_)
         {
             fix_usage();
         }
@@ -113,9 +114,9 @@ namespace DFHack
                       command_function function_,
                       command_hotkey_guard guard_,
                       const char * usage_ = "")
-            : name(_name), description(_description),
-              function(function_), interactive(false),
-              guard(guard_), usage(usage_)
+            : name(_name), description(_description), function(function_),
+              interactive(false), unlocked(false), guard(guard_),
+              usage(usage_)
         {
             fix_usage();
         }
@@ -128,11 +129,12 @@ namespace DFHack
 
         bool isHotkeyCommand() const { return guard != NULL; }
 
-        std::string name;
-        std::string description;
-        command_function function;
-        bool interactive;
-        command_hotkey_guard guard;
+        const std::string name;
+        const std::string description;
+        const command_function function;
+        const bool interactive;
+        const bool unlocked;
+        const command_hotkey_guard guard;
         std::string usage;
     };
     class Plugin

library/modules/Textures.cpp

@@ -322,6 +322,7 @@ struct tracking_stage_new_region : df::viewscreen_new_regionst {
 
     DEFINE_VMETHOD_INTERPOSE(void, logic, ()) {
         if (this->m_raw_load_stage != this->raw_load_stage) {
+            CoreSuspender guard;
             TRACE(textures).print("raw_load_stage %d -> %d\n", this->m_raw_load_stage,
                                   this->raw_load_stage);
             bool tmp_state = loading_state;
@@ -346,6 +347,7 @@ struct tracking_stage_adopt_region : df::viewscreen_adopt_regionst {
 
     DEFINE_VMETHOD_INTERPOSE(void, logic, ()) {
         if (this->m_cur_step != this->cur_step) {
+            CoreSuspender guard;
             TRACE(textures).print("step %d -> %d\n", this->m_cur_step, this->cur_step);
             bool tmp_state = loading_state;
             loading_state = this->cur_step >= 0 && this->cur_step < 3 ? true : false;
@@ -369,6 +371,7 @@ struct tracking_stage_load_region : df::viewscreen_loadgamest {
 
     DEFINE_VMETHOD_INTERPOSE(void, logic, ()) {
         if (this->m_cur_step != this->cur_step) {
+            CoreSuspender guard;
             TRACE(textures).print("step %d -> %d\n", this->m_cur_step, this->cur_step);
             bool tmp_state = loading_state;
             loading_state = this->cur_step >= 0 && this->cur_step < 3 ? true : false;
@@ -392,6 +395,7 @@ struct tracking_stage_new_arena : df::viewscreen_new_arenast {
 
     DEFINE_VMETHOD_INTERPOSE(void, logic, ()) {
         if (this->m_cur_step != this->cur_step) {
+            CoreSuspender guard;
             TRACE(textures).print("step %d -> %d\n", this->m_cur_step, this->cur_step);
             bool tmp_state = loading_state;
             loading_state = this->cur_step >= 0 && this->cur_step < 3 ? true : false;

plugins/examples/persistent_per_save_example.cpp

@@ -170,9 +170,6 @@ DFhackCExport command_result plugin_onupdate(color_ostream &out) {
 }
 
 static command_result do_command(color_ostream &out, vector<string> &parameters) {
-    // be sure to suspend the core if any DF state is read or modified
-    CoreSuspender suspend;
-
     if (!Core::getInstance().isMapLoaded() || !World::IsSiteLoaded()) {
         out.printerr("Cannot run %s without a loaded fort.\n", plugin_name);
         return CR_FAILURE;

plugins/examples/simple_command_example.cpp

@@ -1,12 +1,12 @@
 // This template is appropriate for plugins that simply provide one or more
 // commands, but don't need to be "enabled" to function.
 
-#include <string>
-#include <vector>
-
 #include "Debug.h"
 #include "PluginManager.h"
 
+#include <string>
+#include <vector>
+
 using std::string;
 using std::vector;
 
@@ -32,9 +32,6 @@ DFhackCExport command_result plugin_init(color_ostream &out, std::vector <Plugin
 }
 
 static command_result do_command(color_ostream &out, vector<string> &parameters) {
-    // be sure to suspend the core if any DF state is read or modified
-    CoreSuspender suspend;
-
     // TODO: command logic
 
     return CR_OK;

plugins/examples/skeleton.cpp

@@ -8,11 +8,6 @@
 // configured for specific use cases (but don't come with as many comments as
 // this one does).
 
-#include <string>
-#include <vector>
-
-#include "df/world.h"
-
 #include "Core.h"
 #include "Debug.h"
 #include "MemAccess.h"
@@ -23,6 +18,11 @@
 #include "modules/Screen.h"
 #include "modules/World.h"
 
+#include "df/world.h"
+
+#include <string>
+#include <vector>
+
 using std::string;
 using std::vector;
 
@@ -38,7 +38,8 @@ DFHACK_PLUGIN("skeleton");
 // The identifier declared with this macro (i.e. is_enabled) is used to track
 // whether the plugin is in an "enabled" state. If you don't need enablement
 // for your plugin, you don't need this line. This variable will also be read
-// by the `plug` builtin command; when true the plugin will be shown as enabled.
+// by the `plug` builtin command; when it is true, the plugin will be shown as
+// enabled.
 DFHACK_PLUGIN_IS_ENABLED(is_enabled);
 
 // Any globals a plugin requires (e.g. world) should be listed here.
@@ -80,7 +81,7 @@ DFhackCExport command_result plugin_shutdown(color_ostream &out) {
     DEBUG(status,out).print("shutting down %s\n", plugin_name);
 
     // You *MUST* kill all threads you created before this returns.
-    // If everything fails, just return CR_FAILURE. Your plugin will be
+    // If anything fails, just return CR_FAILURE. Your plugin will be
     // in a zombie state, but things won't crash.
     return CR_OK;
 
@@ -150,7 +151,7 @@ DFhackCExport command_result plugin_onstatechange(color_ostream &out, state_chan
     return CR_OK;
 }
 
-// Whatever you put here will be done in each game frame refresh. Don't abuse it.
+// Whatever you put here will be done in each game tick/frame. Don't abuse it.
 // Note that if the plugin implements the enabled API, this function is only called
 // if the plugin is enabled.
 DFhackCExport command_result plugin_onupdate (color_ostream &out) {
@@ -203,24 +204,21 @@ DFhackCExport command_result plugin_load_site_data (color_ostream &out) {
     return CR_OK;
 }
 
-// This is the callback we registered in plugin_init. Note that while plugin
-// callbacks are called with the core suspended, command callbacks are called
-// from a different thread and need to explicity suspend the core if they
-// interact with Lua or DF game state (most commands do at least one of these).
+// This is the callback we registered in plugin_init. It will be called with the
+// core suspended, just like all the other callbacks. If this function does not
+// interact with Lua or DF game state (most commands do at least one of these),
+// you can call setRunWithCoreUnlocked(true) on the corresponding PluginCommand
+// that is registered in plugin_init.
 static command_result command_callback1(color_ostream &out, vector<string> &parameters) {
     DEBUG(command,out).print("%s command called with %zu parameters\n",
         plugin_name, parameters.size());
 
-    // I'll say it again: always suspend the core in command callbacks unless
-    // all your data is local.
-    CoreSuspender suspend;
-
     // Return CR_WRONG_USAGE to print out your help text. The help text is
     // sourced from the associated rst file in docs/plugins/. The same help will
     // also be returned by 'help your-command'.
 
     // simple commandline parsing can be done in C++, but there are lua libraries
-    // that can easily handle more complex commandlines. see the blueprint plugin
+    // that can easily handle more complex commandlines. see the dwarfvet plugin
     // for an example.
 
     // TODO: do something according to the flags set in the options struct