unitsync abi compatibility, r7064

[Tobi]

Modified: trunk/tools/unitsync/unitsync.cpp
===================================================================
--- trunk/tools/unitsync/unitsync.cpp 2008-11-18 18:38:48 UTC (rev 7063)
+++ trunk/tools/unitsync/unitsync.cpp 2008-11-18 18:40:13 UTC (rev 7064)
@@ -282,12 +282,12 @@
* AddAllArchives() you have to call Init when you want to remove the archives
* from the VFS and start with a clean state.
*/
-DLL_EXPORT int __stdcall Init(bool isServer, int id)
+DLL_EXPORT int __stdcall Init(bool isServer, int id, const char* configFile)
{
try {
_UnInit();

- ConfigHandler::Instantiate("");
+ ConfigHandler::Instantiate(configFile);
FileSystemHandler::Initialize(false);

std::vector<string> filesToCheck;

This breaks unitsync ABI compatibility: configFile is undefined and may be a dangling pointer if a client expects the old ABI, and since unitsync uses stdcall Init will pop the wrong number of arguments from the stack so the caller will probably crash right after Init returns.

Can't Init take a sane default config file and leave switching of config file to SetSpringConfigFile?

+DLL_EXPORT const char* __stdcall GetSpringConfigFile()
+{
+ return configHandler.GetConfigFile().c_str();
+}

Use GetStr() for returning strings, I'm not sure why this was exactly, but I suposse returning .c_str() caused problems in the past since everywhere in unitsync GetStr() has always been used...

[Auswaschbar]

Code: Select all

DLL_EXPORT int          __stdcall Init(bool isServer, int id, const char* filenameAsAbsolutePath="");

So if no argument is passed, it should use the default. I talked with BrainDamage before I added this, and he said it would be best if you have both the Set... function and a parameter in Init. Init() may throw exceptions because of content not found when it loads config from a default location, which doesn't specify datadirs.

[Tobi]

That's C++. It's a C ABI...

Delphi doesn't know about this default argument, for example and will just do, in pseudocode assembler:

Code: Select all

push id
push isServer
call Init

So the filename is some garbage that's on the stack (probably it's function return address or previous content of esp...) instead of empty string.

And when Init returns it will do, in pseudo assembler again,

Code: Select all

pop
pop
pop
return

Popping one too much argument from the stack, screwing up the caller badly.

Init() may throw exceptions because of content not found when it loads config from a default location, which doesn't specify datadirs.

Then it would be better to make it so that SetConfigFile may be called before Init. That way the ABI wouldn't be broken.

(Also it doesn't throw exceptions anymore but neatly sets the error message (for GetLastError) and returns error code nowdays )

[Auswaschbar]

That's C++. It's a C ABI...

This sucks, didn't know about it. An additional reason for not using unitsync in-engine.

So best would be to remove this argument and only initialise confighandler in Init() when it hasn't been previously initialised?

[Tobi]

Yeah suppose so. Also need to take care then that _UnInit doesn't always deinitialize the configHandler, since Init calls _UnInit.

This could be done easily with an argument to _UnInit I suppose.

(_UnInit is the helper for both Init and UnInit, so exception handling is done in the right function. _UnInit is internal so could get extra argument to specify whether configHandler should be uninitialized. Then it could be made to uninitialize configHandler only when called explicitly by the client, and not when called implicitly via a call to Init.)

[Tobi]

Thanks for fixing it Auswaschbar.