Addressed reviewer's comments and bumped scripting engine ABI version

Signed-off-by: Ricardo Dias <[email protected]>

Author: rjd15372

src/commands.def

@@ -5418,7 +5418,7 @@ struct COMMAND_ARG SCRIPT_DEBUG_mode_Subargs[] = {
 /* SCRIPT DEBUG argument table */
 struct COMMAND_ARG SCRIPT_DEBUG_Args[] = {
 {MAKE_ARG("mode",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_NONE,3,NULL),.subargs=SCRIPT_DEBUG_mode_Subargs},
-{MAKE_ARG("engine",ARG_TYPE_STRING,-1,NULL,NULL,"8.1.0",CMD_ARG_OPTIONAL,0,NULL)},
+{MAKE_ARG("engine",ARG_TYPE_STRING,-1,NULL,NULL,"9.0.0",CMD_ARG_OPTIONAL,0,NULL)},
 };
 
 /********** SCRIPT EXISTS ********************/

src/commands/script-debug.json

@@ -39,7 +39,7 @@
                 "name": "engine",
                 "type": "string",
                 "optional": true,
-                "since": "8.1.0"
+                "since": "9.0.0"
             }
         ],
         "reply_schema": {

src/lua/engine_lua.c

@@ -426,6 +426,7 @@ int luaEngineInitEngine(void) {
     ldbInit();
 
     engineMethods methods = {
+        .version = VALKEYMODULE_SCRIPTING_ENGINE_ABI_VERSION,
         .compile_code = luaEngineCompileCode,
         .free_function = luaEngineFreeFunction,
         .call_function = luaEngineFunctionCall,

src/scripting_engine.c

@@ -8,6 +8,7 @@
 #include "dict.h"
 #include "functions.h"
 #include "module.h"
+#include "server.h"
 
 typedef struct scriptingEngineImpl {
     /* Engine specific context */
@@ -76,6 +77,14 @@ size_t scriptingEngineManagerGetMemoryUsage(void) {
     return dictMemUsage(engineMgr.engines) + sizeof(engineMgr);
 }
 
+static inline void scriptingEngineInitializeEngineMethods(scriptingEngine *engine, engineMethods *methods) {
+    if (methods->version < 3) {
+        memcpy(&engine->impl.methods, methods, sizeof(engineMethodsV1));
+    } else {
+        engine->impl.methods = *methods;
+    }
+}
+
 /* Registers a new scripting engine in the engine manager.
  *
  * - `engine_name`: the name of the scripting engine. This name will match
@@ -111,11 +120,11 @@ int scriptingEngineManagerRegister(const char *engine_name,
         .module = engine_module,
         .impl = {
             .ctx = engine_ctx,
-            .methods = *engine_methods,
         },
         .client = c,
         .module_ctx = engine_module ? moduleAllocateContext() : NULL,
     };
+    scriptingEngineInitializeEngineMethods(e, engine_methods);
 
     dictAdd(engineMgr.engines, engine_name_sds, e);
 
@@ -329,6 +338,13 @@ debuggerEnableRet scriptingEngineCallDebuggerEnable(scriptingEngine *engine,
                                                     subsystemType type,
                                                     const debuggerCommand **commands,
                                                     size_t *commands_len) {
+    if (engine->impl.methods.version < 3) {
+        serverLog(LL_WARNING, "Scripting engine '%s' uses ABI version '%lu', which does not support debugger API",
+                  scriptingEngineGetName(engine),
+                  (unsigned long)engine->impl.methods.version);
+        return VMSE_DEBUG_NOT_SUPPORTED;
+    }
+
     if (engine->impl.methods.debugger_enable == NULL ||
         engine->impl.methods.debugger_disable == NULL ||
         engine->impl.methods.debugger_start == NULL ||
@@ -349,6 +365,9 @@ debuggerEnableRet scriptingEngineCallDebuggerEnable(scriptingEngine *engine,
 
 void scriptingEngineCallDebuggerDisable(scriptingEngine *engine,
                                         subsystemType type) {
+    serverAssert(engine->impl.methods.version >= 3);
+    serverAssert(engine->impl.methods.debugger_disable != NULL);
+
     engineSetupModuleCtx(engine, NULL);
     engine->impl.methods.debugger_disable(
         engine->module_ctx,
@@ -360,6 +379,9 @@ void scriptingEngineCallDebuggerDisable(scriptingEngine *engine,
 void scriptingEngineCallDebuggerStart(scriptingEngine *engine,
                                       subsystemType type,
                                       robj *source) {
+    serverAssert(engine->impl.methods.version >= 3);
+    serverAssert(engine->impl.methods.debugger_start != NULL);
+
     engineSetupModuleCtx(engine, NULL);
     engine->impl.methods.debugger_start(
         engine->module_ctx,
@@ -371,6 +393,9 @@ void scriptingEngineCallDebuggerStart(scriptingEngine *engine,
 
 void scriptingEngineCallDebuggerEnd(scriptingEngine *engine,
                                     subsystemType type) {
+    serverAssert(engine->impl.methods.version >= 3);
+    serverAssert(engine->impl.methods.debugger_end != NULL);
+
     engineSetupModuleCtx(engine, NULL);
     engine->impl.methods.debugger_end(
         engine->module_ctx,
@@ -450,6 +475,11 @@ int scriptingEngineDebuggerEnable(client *c, scriptingEngine *engine, sds *err)
  * to properly shut down a client debugging session, see scriptingEngineDebuggerEndSession()
  * for more information. */
 void scriptingEngineDebuggerDisable(client *c) {
+    if (ds.engine == NULL) {
+        /* No debug session enabled. */
+        return;
+    }
+
     ds.commands = NULL;
     ds.commands_len = 0;
     c->flag.lua_debug = 0;
@@ -569,6 +599,8 @@ int scriptingEngineDebuggerStartSession(client *c) {
 /* End a debugging session after the EVAL call with debugging enabled
  * returned. */
 void scriptingEngineDebuggerEndSession(client *c) {
+    serverAssert(ds.active);
+
     /* Emit the remaining logs and an <endsession> mark. */
     scriptingEngineDebuggerLog(sdsnew("<endsession>"));
     scriptingEngineDebuggerFlushLogs();

src/scripting_engine.h

@@ -2,6 +2,7 @@
 #define _SCRIPTING_ENGINE_H_
 
 #include "server.h"
+#include "valkeymodule.h"
 
 // Forward declaration of the engine structure.
 typedef struct scriptingEngine scriptingEngine;
@@ -17,6 +18,7 @@ typedef ValkeyModuleScriptingEngineCallableLazyEvalReset callableLazyEvalReset;
 typedef ValkeyModuleScriptingEngineDebuggerEnableRet debuggerEnableRet;
 typedef ValkeyModuleScriptingEngineDebuggerCommand debuggerCommand;
 typedef ValkeyModuleScriptingEngineDebuggerCommandParam debuggerCommandParam;
+typedef ValkeyModuleScriptingEngineMethodsV1 engineMethodsV1;
 typedef ValkeyModuleScriptingEngineMethods engineMethods;
 
 /*

src/valkey-cli.c

@@ -3524,7 +3524,7 @@ static int evalMode(int argc, char **argv) {
 
         char *engine_name = NULL;
         if (script[0] == '#' && script[1] == '!') {
-            const char *sp = strchr(script, '\n');
+            const char *sp = strpbrk(script, "\r\n ");
             engine_name = strndup(script + 2, (sp - script) - 2);
         } else {
             engine_name = strdup("lua");

src/valkeymodule.h

@@ -1119,7 +1119,12 @@ typedef void (*ValkeyModuleScriptingEngineDebuggerEndFunc)(
     ValkeyModuleScriptingEngineSubsystemType type);
 
 /* Current ABI version for scripting engine modules. */
-#define VALKEYMODULE_SCRIPTING_ENGINE_ABI_VERSION 2UL
+/* Version Changelog:
+ *  - 1: Initial version.
+ *  - 2: Changed the `compile_code` callback to support binary data in the source code.
+ *  - 3: Added support for new debugging commands.
+ */
+#define VALKEYMODULE_SCRIPTING_ENGINE_ABI_VERSION 3UL
 
 typedef struct ValkeyModuleScriptingEngineMethods {
     uint64_t version; /* Version of this structure for ABI compat. */
@@ -1148,6 +1153,35 @@ typedef struct ValkeyModuleScriptingEngineMethods {
     /* Function callback to get the used memory by the engine. */
     ValkeyModuleScriptingEngineGetMemoryInfoFunc get_memory_info;
 
+} ValkeyModuleScriptingEngineMethodsV1;
+
+typedef struct ValkeyModuleScriptingEngineMethodsV2 {
+    uint64_t version; /* Version of this structure for ABI compat. */
+
+    /* Compile code function callback. When a new script is loaded, this
+     * callback will be called with the script code, compiles it, and returns a
+     * list of `ValkeyModuleScriptingEngineCompiledFunc` objects. */
+    union {
+        ValkeyModuleScriptingEngineCompileCodeFuncV1 compile_code_v1;
+        ValkeyModuleScriptingEngineCompileCodeFunc compile_code;
+    };
+    /* Function callback to free the memory of a registered engine function. */
+    ValkeyModuleScriptingEngineFreeFunctionFunc free_function;
+
+    /* The callback function called when `FCALL` command is called on a function
+     * registered in this engine. */
+    ValkeyModuleScriptingEngineCallFunctionFunc call_function;
+
+    /* Function callback to return memory overhead for a given function. */
+    ValkeyModuleScriptingEngineGetFunctionMemoryOverheadFunc get_function_memory_overhead;
+
+    /* The callback function used to reset the runtime environment used
+     * by the scripting engine for EVAL scripts. */
+    ValkeyModuleScriptingEngineResetEvalEnvFunc reset_eval_env;
+
+    /* Function callback to get the used memory by the engine. */
+    ValkeyModuleScriptingEngineGetMemoryInfoFunc get_memory_info;
+
     /* Function callback to enable the debugger for the future execution of scripts. */
     ValkeyModuleScriptingEngineDebuggerEnableFunc debugger_enable;
 
@@ -1161,9 +1195,9 @@ typedef struct ValkeyModuleScriptingEngineMethods {
     ValkeyModuleScriptingEngineDebuggerEndFunc debugger_end;
 
 
-} ValkeyModuleScriptingEngineMethodsV1;
+} ValkeyModuleScriptingEngineMethodsV2;
 
-#define ValkeyModuleScriptingEngineMethods ValkeyModuleScriptingEngineMethodsV1
+#define ValkeyModuleScriptingEngineMethods ValkeyModuleScriptingEngineMethodsV2
 
 /* ------------------------- End of common defines ------------------------ */