Ensure custom preferences path ends with path separator

[daftmugi]

If the user launches FSO with a FSO_PREFERENCES_PATH value that does not end with a path separator (Linux /, Windows \), FSO will create a fs2_open.ini file in the wrong location.

Example:
Given: FSO_PREFERENCES_PATH='./config' fs2_open
Path is: './config'
INI file path becomes: ./configfs2_open.ini

This PR ensures that the custom preferences path ends with a path separator, resulting in the fs2_open.ini file being created in the expected location.

Example with PR:
Given: FSO_PREFERENCES_PATH='./config' fs2_open
Path becomes: './config/'
INI file path becomes: ./config/fs2_open.ini

Extends #6387.

[daftmugi]

I force pushed a different take on this PR.

Originally, I merely added a path separator to the end of the path, if there wasn't one on FSO_PREFERENCES_PATH. However, on Linux, I was getting a free(): invalid pointer at program exit.

The following code was trying to SDL_free() the value set by the FSO_PREFERENCES_PATH code path.

void os_deinit() {
  if (preferencesPath) {
    SDL_free(preferencesPath);
    preferencesPath = nullptr;
  }
}

I decided to rewrite this PR to address that at the same time. I changed the type of preferencesPath from char* to SCP_string.

Of note, there's a #ifdef WIN32 block that could use some extra eyes to make sure it was changed correctly.

[Shivansps]

yeah i was thinking about do it myself as well.

That said, i think the problem was actually caused because some internal part of the code is calling getPreferencesPath() directly when it should be done via os_get_config_path()

fs2open.github.com/code/osapi/osapi.cpp

Line 815 in 440eed6

SCP_string os_get_config_path(const SCP_string& subpath)

Because if you do this and pass the path without the ending directory separator the pilots profiles are actually writen correctly, its the fs2_open.inis that are not.

But since there is no coment or indicator that you cant use getPreferencesPath() directly, i think this is the right thing to do.

Also, i might be wrong, i didnt do enoght testing in this case.

[Goober5000]

Yeah there are several places in the code that call getPreferencesPath() directly.

Also keep in mind that os_get_config_path() calls os_is_legacy_mode(), which calls getPreferencesPath(). If this is changed to os_get_config_path(), you'd get an endless loop. (There might be a partial endless loop even now, since there is a comment about infinite recursion.)

[daftmugi]

Just to clarify, are there any changes that I need to make?

[Shivansps]

Then i think checking for the ending directory terminator on getPreferencesPath() is the right thing to do.

[Goober5000]

Just to clarify, are there any changes that I need to make?

Mainly to resolve this question:

Regarding @Shivansps 's comment:

That said, i think the problem was actually caused because some internal part of the code is calling getPreferencesPath() directly when it should be done via os_get_config_path()

Is this actually the case? The only places in the code where getPreferencesPath() is called is inside os_get_config_path() itself, as well as inside os_is_legacy_mode().

If os_is_legacy_mode() is modified to call os_get_config_path(), then that would cause an endless loop, which would need to be resolved somehow.

EDIT: I just looked over the PR. In other respects, the changes look good.

[daftmugi]

Just leaving some notes here for future reference.

The pilot files get saved in the correct location regardless if the preferences path has a trailing slash, because the trailing slash is added if it's not present.

// - pilotfile::save_player(...)
//     - cfopen(filename, ..., CF_TYPE_PLAYERS, ...)
//         - _cfopen(filename, ..., CF_TYPE_PLAYERS, ...)
//             - cf_create_default_path_string(..., pathtype, filename, ...)
////////////////////////////////////////////////////////////////////////////

// Don't add slash for root directory
if (Pathtypes[pathtype].path[0] != '\0') {
    if (path.back() != DIR_SEPARATOR_CHAR) {
        path += DIR_SEPARATOR_CHAR;
    }
}

// add filename
if (filename) {
    path += filename;
}

The fs2_open.ini file having the incorrect path when missing a trailing slash is due to there being no check for the trailing slash or assuming that the trailing slash is guaranteed.

// - os_init_registry_stuff(...)
//     - profile_read(Osreg_config_file_name /* "fs2_open.ini" */)
//         - os_get_config_path(...)
////////////////////////////////////////////////////////////////////////////

ss << getPreferencesPath() << compatiblePath;

[Shivansps]

Just to clarify, are there any changes that I need to make?

Mainly to resolve this question:

Regarding @Shivansps 's comment:

That said, i think the problem was actually caused because some internal part of the code is calling getPreferencesPath() directly when it should be done via os_get_config_path()

Is this actually the case? The only places in the code where getPreferencesPath() is called is inside os_get_config_path() itself, as well as inside os_is_legacy_mode().

If os_is_legacy_mode() is modified to call os_get_config_path(), then that would cause an endless loop, which would need to be resolved somehow.

EDIT: I just looked over the PR. In other respects, the changes look good.

To clarify, what i meant to say is that getPrefencesPath() is not meant to be called from outside "osapi.cpp", looking back at the initial PRs i think it is just meant as a optimization so SDL_GetPrefPath() is not called multiples times because its slow.
Looking closely at the header file it cant, because its not there. So this is good.

daftmurgi is correct

i got confused because of this line

fs2open.github.com/code/osapi/osapi.cpp

Line 840 in 440eed6

ss << DIR_SEPARATOR_CHAR << compatiblePath;

os_get_config_path() does not add the dir char separator unless it is in legacy mode, as i mistakely belived.