Add an abstraction for hooking v1 and v2 commands

This is a step towards making command hooks work in OE, once OE is
supported.

Not the most ideal or efficient thing in the world, but it works okay
until we come up with something better, I suppose. Not a fan of the argv
copying but avoiding that would make the API a lot less ergonomic.

Not the easiest problem to solve, really...

4 files changed, 130 insertions, 50 deletions

diff --git a/src/con_.h b/src/con_.h
index c2c0cce..b67483a 100644
--- a/src/con_.h
+++ b/src/con_.h
@@ -361,6 +361,66 @@ extern struct _con_vtab_iconvar_wrap {
 #define DEF_CCMD_PLUSMINUS_UNREG DEF_CCMD_PLUSMINUS
 
 /*
+ * Defines a hook function in-place to hook a command callback, factoring in
+ * different callback ABIs used by different commands and/or different engine
+ * branches. Defines a hook_##name##_cb function to install the hook and an
+ * unhook_##name##_cb function to remove it.
+ *
+ * The hook function has the implicit arguments argc and argv, just like a
+ * command handler defined with DEF_CCMD_HERE. Calling the original command
+ * handler can be done using orig_##name##_cb, passing through argc and argv.
+ *
+ * Note that argc and argv MUST remain unmodified, as not all callback
+ * interfaces pass arguments through the callback and so attempting to change
+ * these parameters could cause unexpected or inconsistent behaviour across
+ * engine versions.
+ *
+ * In some cases, a command will be defined to take no arguments, in which case
+ * argc will be zero and argv will be null. In these cases, the parameters
+ * should still be passed through to the orig_ function, as this ensures
+ * compatibility with other game/engine versions.
+ */
+#define DEF_CCMD_COMPAT_HOOK(name) \
+	static union { \
+		con_cmdcbv1 v1; \
+		con_cmdcbv2 v2; \
+	} _orig_##name##_cb; \
+	static void _orig_##name##_cbv1(int argc, const char *const *argv) { \
+		_orig_##name##_cb.v1(); \
+	} \
+	static void _orig_##name##_cbv2(int argc, const char *const *argv) { \
+		struct con_cmdargs args; \
+		args.argc = argc; \
+		/* XXX: having to copy argv sucks, but can't see how to avoid without
+		   ruining the interface? */ \
+		for (int i = 0; i < argc; ++i) args.argv[i] = argv[i]; \
+		_orig_##name##_cb.v2(&args); \
+	} \
+	static void (*orig_##name##_cb)(int argc, const char *const *argv); \
+	static void _hook_##name##_cb(int argc, const char *const *argv); \
+	static void _hook_##name##_cbv1() { \
+		_hook_##name##_cb(0, 0); /* XXX: ??? */ \
+	} \
+	static void _hook_##name##_cbv2(const struct con_cmdargs *args) { \
+		_hook_##name##_cb(args->argc, args->argv); \
+	} \
+	static void hook_##name##_cb(struct con_cmd *cmd) { \
+		_orig_##name##_cb.v1 = cmd->cb_v1; \
+		if (cmd->use_newcb) { \
+			cmd->cb_v2 = &_hook_##name##_cbv2; \
+			orig_##name##_cb = &_orig_##name##_cbv2; \
+		} \
+		else { \
+			cmd->cb_v1 = _hook_##name##_cbv1; \
+			orig_##name##_cb = &_orig_##name##_cbv1; \
+		} \
+	} \
+	static void unhook_##name##_cb(struct con_cmd *cmd) { \
+		cmd->cb_v1 = _orig_##name##_cb.v1; \
+	} \
+	static void _hook_##name##_cb(int argc, const char *const *argv) /* ... */
+
+/*
  * These functions register a command or variable, respectively, defined with
  * the _UNREG variants of the above macros. These can be used to conditionally
  * register things. Wherever possible, it is advised to use the DEF_FEAT_*
diff --git a/src/demorec.c b/src/demorec.c
index 6f1b2a7..a763176 100644
--- a/src/demorec.c
+++ b/src/demorec.c
@@ -101,16 +101,15 @@ static void VCALLCONV hook_StopRecording(struct CDemoRecorder *this) {
 DECL_VFUNC_DYN(struct CDemoRecorder, void, StartRecording)
 
 static struct con_cmd *cmd_record, *cmd_stop;
-static con_cmdcbv2 orig_record_cb, orig_stop_cb;
 
-static void hook_record_cb(const struct con_cmdargs *args) {
+DEF_CCMD_COMPAT_HOOK(record) {
 	if_cold (!CHECK_DemoControlAllowed()) return;
 	bool was = *recording;
-	if (!was && args->argc == 2 || args->argc == 3) {
+	if (!was && argc == 2 || argc == 3) {
 		// safety check: make sure a directory exists, otherwise recording
 		// silently fails. this is necessarily TOCTOU, but in practice it's
 		// way better than not doing it - just to have a sanity check.
-		const char *arg = args->argv[1];
+		const char *arg = argv[1];
 		const char *lastslash = 0;
 		for (const char *p = arg; *p; ++p) {
 #ifdef _WIN32
@@ -155,7 +154,7 @@ static void hook_record_cb(const struct con_cmdargs *args) {
 			}
 		}
 	}
-	orig_record_cb(args);
+	orig_record_cb(argc, argv);
 	if (!was && *recording) {
 		*demonum = 0; // see SetSignonState comment above
 		// For UX, make it more obvious we're recording, in particular when not
@@ -166,16 +165,15 @@ static void hook_record_cb(const struct con_cmdargs *args) {
 	EMIT_DemoRecordStarting();
 }
 
-static void hook_stop_cb(const struct con_cmdargs *args) {
+DEF_CCMD_COMPAT_HOOK(stop) {
 	if_cold (!CHECK_DemoControlAllowed()) return;
 	wantstop = true;
-	orig_stop_cb(args);
+	orig_stop_cb(argc, argv);
 	wantstop = false;
 }
 
-static inline bool find_demorecorder() {
+static inline bool find_demorecorder(const uchar *insns) {
 #ifdef _WIN32
-	const uchar *insns = (const uchar *)orig_stop_cb;
 	// The stop command loads `demorecorder` into ECX to call IsRecording()
 	for (const uchar *p = insns; p - insns < 32;) {
 		if (p[0] == X86_MOVRMW && p[1] == X86_MODRM(0, 1, 5)) {
@@ -238,8 +236,8 @@ bool demorec_start(const char *name) {
 	if (was) return false;
 	// dumb but easy way to do this: call the record command callback. note:
 	// this args object is very incomplete by enough to make the command work
-	struct con_cmdargs args = {.argc = 2, .argv = {0, name, 0}};
-	orig_record_cb(&args);
+	// TODO(compat): will this be a problem for OE with the global argc/argv?
+	orig_record_cb(2, (const char *[]){0, name});
 	if (!was && *recording) *demonum = 0; // same logic as in the hook
 	EMIT_DemoRecordStarting();
 	return *recording;
@@ -260,10 +258,8 @@ int demorec_demonum() {
 
 INIT {
 	cmd_record = con_findcmd("record");
-	orig_record_cb = con_getcmdcbv2(cmd_record);
 	cmd_stop = con_findcmd("stop");
-	orig_stop_cb = con_getcmdcbv2(cmd_stop);
-	if_cold (!find_demorecorder()) {
+	if_cold (!find_demorecorder(cmd_stop->cb_insns)) {
 		errmsg_errorx("couldn't find demo recorder instance");
 		return FEAT_INCOMPAT;
 	}
@@ -281,15 +277,12 @@ INIT {
 		errmsg_errorx("couldn't find demo basename variable");
 		return FEAT_INCOMPAT;
 	}
-
 	orig_SetSignonState = (SetSignonState_func)hook_vtable(vtable,
 			vtidx_SetSignonState, (void *)&hook_SetSignonState);
 	orig_StopRecording = (StopRecording_func)hook_vtable(vtable,
 			vtidx_StopRecording, (void *)&hook_StopRecording);
-
-	cmd_record->cb_v2 = &hook_record_cb;
-	cmd_stop->cb_v2 = &hook_stop_cb;
-
+	hook_record_cb(cmd_record);
+	hook_stop_cb(cmd_stop);
 	return FEAT_OK;
 }
 
@@ -300,8 +293,8 @@ END {
 	void **vtable = demorecorder->vtable;
 	unhook_vtable(vtable, vtidx_SetSignonState, (void *)orig_SetSignonState);
 	unhook_vtable(vtable, vtidx_StopRecording, (void *)orig_StopRecording);
-	cmd_record->cb_v2 = orig_record_cb;
-	cmd_stop->cb_v2 = orig_stop_cb;
+	unhook_record_cb(cmd_record);
+	unhook_stop_cb(cmd_stop);
 }
 
 // vi: sw=4 ts=4 noet tw=80 cc=80
diff --git a/src/portalisg.c b/src/portalisg.c
index b9a85d9..bdc007f 100644
--- a/src/portalisg.c
+++ b/src/portalisg.c
@@ -36,6 +36,7 @@ static con_cmdcbv2 disconnect_cb;
 
 DEF_FEAT_CCMD_HERE(sst_portal_resetisg,
 		"Remove \"ISG\" state and disconnect from the server", 0) {
+	// TODO(compat): OE? guess it might work by accident due to cdecl, find out
 	disconnect_cb(&(struct con_cmdargs){0});
 	*isg_flag = false;
 }
diff --git a/src/sst.c b/src/sst.c
index 08e2b3e..f1b8241 100644
--- a/src/sst.c
+++ b/src/sst.c
@@ -418,7 +418,6 @@ DEF_EVENT(PluginLoaded)
 DEF_EVENT(PluginUnloaded)
 
 static struct con_cmd *cmd_plugin_load, *cmd_plugin_unload;
-static con_cmdcbv2 orig_plugin_load_cb, orig_plugin_unload_cb;
 
 static int ownidx; // XXX: super hacky way of getting this to do_unload()
 
@@ -431,36 +430,50 @@ static bool ispluginv1(const struct CPlugin *plugin) {
 			plugin->v1.ifacever;
 }
 
-static void hook_plugin_load_cb(const struct con_cmdargs *args) {
-	if (args->argc > 1 && !CHECK_AllowPluginLoading(true)) return;
+DEF_CCMD_COMPAT_HOOK(plugin_load) {
+	if (argc > 1 && !CHECK_AllowPluginLoading(true)) return;
 	int prevnplugins = pluginhandler->plugins.sz;
-	orig_plugin_load_cb(args);
+	orig_plugin_load_cb(argc, argv);
 	// note: if loading fails, we won't see an increase in the plugin count.
 	// we of course only want to raise the PluginLoaded event on success
 	if (pluginhandler->plugins.sz != prevnplugins) EMIT_PluginLoaded();
 }
-static void hook_plugin_unload_cb(const struct con_cmdargs *args) {
-	if (args->argc > 1) {
-		if (!CHECK_AllowPluginLoading(false)) return;
-		if (!*args->argv[1]) {
+
+// plugin_unload hook is kind of special. We have to force a tail call when
+// unloading SST itself, so we can't use the regular hook wrappers. Instead we
+// need to specifically and directly hook the two callback interfaces.
+static union {
+	con_cmdcbv1 v1;
+	con_cmdcbv2 v2;
+} orig_plugin_unload_cb;
+
+enum unload_action {
+	UNLOAD_SKIP,
+	UNLOAD_SELF,
+	UNLOAD_OTHER
+};
+static int hook_plugin_unload_common(int argc, const char *const *argv) {
+	if (argc > 1) {
+		if (!CHECK_AllowPluginLoading(false)) return UNLOAD_SKIP;
+		if (!*argv[1]) {
 			errmsg_errorx("plugin_unload expects a number, got an empty string");
-			return;
+			return UNLOAD_SKIP;
 		}
 		// catch the very common user error of plugin_unload <name> and try to
 		// hint people in the right direction. otherwise strings get atoi()d
 		// silently into zero, which is confusing and unhelpful. don't worry
 		// about numeric range/overflow, worst case scenario it's a sev 9.8 CVE.
 		char *end;
-		int idx = strtol(args->argv[1], &end, 10);
-		if (end == args->argv[1]) {
+		int idx = strtol(argv[1], &end, 10);
+		if (end == argv[1]) {
 			errmsg_errorx("plugin_unload takes a number, not a name");
 			errmsg_note("use plugin_print to get a list of plugin indices");
-			return;
+			return UNLOAD_SKIP;
 		}
 		if (*end) {
 			errmsg_errorx("unexpected trailing characters "
 					"(plugin_unload takes a number)");
-			return;
+			return UNLOAD_SKIP;
 		}
 		struct CPlugin **plugins = pluginhandler->plugins.m.mem;
 		if_hot (idx >= 0 && idx < pluginhandler->plugins.sz) {
@@ -471,26 +484,39 @@ static void hook_plugin_unload_cb(const struct con_cmdargs *args) {
 			if (common->theplugin == &plugin_obj) {
 				sst_userunloaded = true;
 				ownidx = idx;
+				return UNLOAD_SELF;
+			}
+			// if it's some other plugin being unloaded, we can keep doing stuff
+			// after, so we raise the event.
+			return UNLOAD_OTHER;
+		}
+	}
+	// error case, pass through to original to log the appropriate message
+	return UNLOAD_OTHER;
+}
+
+// TODO(compat): we'd have cbv1 here for OE. but we don't yet have the global
+// argc/argv access so there's no way to implement that.
+
+static void hook_plugin_unload_cbv2(const struct con_cmdargs *args) {
+	int action = hook_plugin_unload_common(args->argc, args->argv);
+	switch_exhaust_enum(unload_action, action) {
+		case UNLOAD_SKIP:
+			return;
+		case UNLOAD_SELF:
 #ifdef __clang__
 			// thanks clang for forcing use of return here and ALSO warning!
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wpedantic"
-			__attribute__((musttail)) return orig_plugin_unload_cb(args);
+			__attribute__((musttail)) return orig_plugin_unload_cb.v2(args);
 #pragma clang diagnostic pop
 #else
 #error We are tied to clang without an assembly solution for this!
 #endif
-			}
-			// if it's some other plugin being unloaded, we can keep doing stuff
-			// after, so we raise the event.
-			orig_plugin_unload_cb(args);
+		case UNLOAD_OTHER:
+			orig_plugin_unload_cb.v2(args);
 			EMIT_PluginUnloaded();
-			return;
-		}
 	}
-	// if the index is either unspecified or out-of-range, let the original
-	// handler produce an appropriate error message
-	orig_plugin_unload_cb(args);
 }
 
 static bool do_load(ifacefactory enginef, ifacefactory serverf) {
@@ -517,11 +543,11 @@ static bool do_load(ifacefactory enginef, ifacefactory serverf) {
 	if (!deferinit()) { do_featureinit(); fixes_apply(); }
 	if_hot (pluginhandler) {
 		cmd_plugin_load = con_findcmd("plugin_load");
-		orig_plugin_load_cb = cmd_plugin_load->cb_v2;
-		cmd_plugin_load->cb_v2 = &hook_plugin_load_cb;
+		hook_plugin_load_cb(cmd_plugin_load);
 		cmd_plugin_unload = con_findcmd("plugin_unload");
-		orig_plugin_unload_cb = cmd_plugin_unload->cb_v2;
-		cmd_plugin_unload->cb_v2 = &hook_plugin_unload_cb;
+		orig_plugin_unload_cb.v1 = cmd_plugin_unload->cb_v1; // n.b.: union!
+		// TODO(compat): detect OE/v1 and use that hook here when required
+		cmd_plugin_unload->cb_v2 = &hook_plugin_unload_cbv2;
 	}
 	return true;
 }
@@ -529,8 +555,8 @@ static bool do_load(ifacefactory enginef, ifacefactory serverf) {
 static void do_unload() {
 	// slow path: reloading shouldn't happen all the time, prioritise fast exit
 	if_cold (sst_userunloaded) { // note: if we're here, pluginhandler is set
-		cmd_plugin_load->cb_v2 = orig_plugin_load_cb;
-		cmd_plugin_unload->cb_v2 = orig_plugin_unload_cb;
+		unhook_plugin_load_cb(cmd_plugin_load);
+		cmd_plugin_unload->cb_v1 = orig_plugin_unload_cb.v1;
 #ifdef _WIN32 // this bit is only relevant in builds that predate linux support
 		struct CPlugin **plugins = pluginhandler->plugins.m.mem;
 		// see comment in CPlugin struct. setting this to the real handle right