Ensure that decode/encode functions use the locale.

[jjceresa]

This PR fixes issue #37

1)Each decode/encode functions use the locale as needed. On return, the locale is restored to the value it had before calling any function.

Using swami, the decode functions are called at swami initialization when loading preference. It is best to have fluidsynth plugin that load and decode fluidsynth's properties from the xml preference file. Also, before exiting swami, it is useful to change some doublevalues (i.e reverb, chorus knobs) to ensure that the change are coded and saved to the preference file. Tested on Windows only.

2)Also IpatchXmlObject.c module has been documented.

[derselbst]

One thing to note: On linux the "C" locale is specified. I guess that's a leftover from my code and you intended ""?

[jjceresa]

On Windows the "C" locale doesn't work in decode/encode function, but "" locale works fine. If you prefer we can specify ""for linux too (if it works). Indeed it is better to have things identical for both OS.

[derselbst]

Well, actually I prefer "C", because "" depends on other environment variables which are implementation defined. Didn't know that "C" doesn't work on Windows.

[realnc]

On POSIX, "" means that the locale will be set to the user's system locale. That means that floating point decimals will need to adhere to that. On a French system, , has to be used and . won't work. To always use . as decimal separator on all systems, LC_NUMERIC needs to be set to "C".

From what I know, this should be the same on Windows though. Weird that it doesn't appear to be the case here :-/

[realnc]

I took a look at the code and it seems it's not possible to do this safely on Windows. _configthreadlocale just doesn't solve the problem because between the point of storing the old locale and restoring it later, another thread might change the global application locale and as a result the old locale that the code restores at the end will no longer be the correct one.

The POSIX way doesn't seem to have that issue, because it offers LC_GLOBAL_LOCALE, which Windows doesn't.

There's a second race condition in the code itself, because saving and restoring the _configthreadlocale status is not done in a thread safe manner. Note that _configthreadlocale returns the old status when setting a new one, which allows us to do this operation atomically, so the correct way is:

    int oldSetting = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
    g_return_val_if_fail(oldSetting != -1, FALSE);
    char* oldLocale = setlocale(LC_NUMERIC, NULL);
    g_return_val_if_fail(setlocale(LC_NUMERIC, "") != NULL, FALSE);

This solves this specific race condition, but it doesn't solve the other one, which can't be solved. _configthreadlocale is just not good enough.

The POSIX path has the same race condition in the code itself, and also note that uselocale returns the old locale when setting a new one. So to fix it:

    locale_t newLocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
    g_return_val_if_fail(newLocale != (locale_t) 0, FALSE);
    locale_t oldLocale = uselocale(newLocale);
    g_return_val_if_fail(oldLocale != (locale_t) 0, FALSE);

Unlike the Windows path, this should be fully thread safe.

[jjceresa]

Thanks for your feedback,

On POSIX, "" means that the locale will be set to the user's system locale. That means that floating point decimals will need to adhere to that.On a French system ', ' has to be used and '.' won't work.

This is this behaviour that is expected from theencode/decode functions. That is: 1)encode is expected to produce a result file setting based on user's system locale. 2)then decode is expected to do the reverse on the same system.

To always use . as decimal separator on all systems, LC_NUMERIC needs to be set to "C".

As far I understand, Iibinstpatch encode/decode functions are not intended to produce a result file setting usable on all systems. The scope of this result file is limited on the same system that produce this file. In other words, an encoded setting file result produced on a French system (for example) isn't intended to be decoded on any system using an unknown user's locale.

[jjceresa]

There's a second race condition in the code itself, because saving and restoring the _configthreadlocale status is not done in a thread safe manner. Note that _configthreadlocale returns the old status when setting a new one, which allows us to do this operation atomically....

Thanks for pointed this!

[realnc]

Thanks for pointed this!

I made a mistake in my explanation :P It wasn't the saving/restoring of the _configthreadlocale status that wasn't thread-safe, it was the storing of the current locale (it was done prior to setting _ENABLE_PER_THREAD_LOCALE as the new status.

So the code is the correct fix for the wrong explanation :P

[realnc]

As far I understand, Iibinstpatch encode/decode functions are not intended to produce a result file setting usable on all systems. The scope of this result file is limited on the same system that produce this file.

Hm. In this case it might be appropriate to not set a locale at all. Instead, different conversion functions could be used instead that take the wanted locale as a parameter (in this case, "C" would be passed to them unconditionally.) Windows, there's _strtod_l and friends. On POSIX there isn't anything like that, but there seem to exist BSD functions like strtod_l that also take a locale argument. Enlightenment's EFL for example does it that way. This might be a good starting point:

https://github.com/Enlightenment/efl/commit/33b8e5157a25b73a0729055ecd22b17e97781e11

[derselbst]

@realnc Thanks for pointing out the race conditions.

strtod_l looks good. Unfortunately, those functions are unportable, as the do not seem to conform to a standard. Which is why I have a slight preference for the current setlocale solution.

[realnc]

I assume using C++ is out of the question? It provides standard functions like std::num_get that operate with a specific locale you give them.

[derselbst]

IMO, C++ as being the "bazooka" is not appropriate in this case. I would rather use strtod_l and risk that the build breaks for somebody.

[jjceresa]

@realnc, please it would be great if you could check that recent commit of this PR e042a7d, fixes the issue https://github.com/swami/libinstpatch/issues/37 you noticed in breakage: realnc/dosbox-core#7 ?

[realnc]

please it would be great if you could check that recent commit of this PR e042a7d, fixes the issue #37 you noticed in breakage: realnc/dosbox-core#7 ?

It does on Linux. Can't test on Windows.

[jjceresa]

It does on Linux.

Thanks.

Can't test on Windows.

On Windows, tested using Swami application only, this PR works as before.

[derselbst]

Great! I would merge this then.

@realnc Thanks for bringing this up!

[jjceresa]

Great! I would merge this then.

Ok, thanks.