Implementation of platform-independ environment manipulations #1272

Merged
merged 2 commits into from May 11, 2013

Conversation

Projects
None yet
7 participants
Member

Karlson2k commented Aug 10, 2012

While debugging of XBMC, I've discovered that win32 environment manipulation isn't working at all.
The problem is that win32 functions SetEnvironmentVariable/GetEnvironmentVariable and runtime functions getenv/putenv operate with separate copies of Environment. So after set (for example) of 'XBMC_HOME' with SetEnvironmentVariable, next call to getenv for 'XBMC_HOME' returns error (variable not set).
Other problem on win32 platform, that XBMC use UTF-8 for CStdString, but when it converted to 'char *' win32 use it as ANSI or OEM encoding. That can cause problems if variable value contains some national characters, as paths variables.
One more problem for win32 - different runtime libraries for third-party modules, which should be updated separately. This is done partially by win32env.c, but that implementation is buggy (new loaded libraries isn't checked, can potentially crash after library unload).
This implementation is uniform for all platforms, so we can drop a lot of "ifdef"s in code.
All UTF-8<->wide conversion is carefully implemented internally where is't needed.
Almost all strings forms used in XBMC is supported as arguments.

This PR is adds new functions support and only a few function uses (as a test), and only for win32 platform.
If it's OK, I'll create more PRs with total replace for setenv/getenv functions.

Member

wsoltys commented Aug 10, 2012

I like the idea in general but I dunno why

  1. we need so much code and so many new functions
  2. charsetconverter isn't used

For 2) needs to be taken care of as we use it everywhere and if there's something wrong for windows it should be fixed there. Also please replace _WIN32 by TARGET_WINDOWS.
At last I dunno if Linux/OSX boys are willing to add extra code for existing functions on their platform even though we might get rid of some ifdefs ;)

Member

Karlson2k commented Aug 10, 2012

@wsoltys Answers: :)

  1. Basically I was going to implement only 'char ', 'wchar_t *', 'CStdStringA' and 'CStdStringW' versions. All those type of variables are used in XBMC and some function need internally CStdStringX versions, some - only Xchar version, so to avoid unnecessary char<->CStdString conversions I implement 4 versions of functions.
    But then I got an error when I tried to compile a simple command like "xbmc_setenv("XBMC_HOME", cstdstringvar). Compiler didn't know which functions should be used "xbmc_setenv(char *, char *)" or "xbmc_setenv(CStdStringA&, CStdStringA&)" as both conversion is possible. So I decided to implement all possible versions, trying each time to use less possible conversions.
    Anyway, this give a great flexibility on using this functions and it's not too hard to maintain all those code as huge part of functions is implemented by calling other similar functions.

  2. charsetconverter is used for non-windows platform. But charset conversion is needed mostly for Windows. So I decided to use direct Windows functions for two reasons:

  1. When coding for Windows, native Windows conversion should be faster.
  2. This is the only way to check conversion success, as XBMC conversion functions in case of fail just return original string converted via CStdString conversion (so converted as ANSI -> wide or vice versa). In case that string contain illegal ANSI characters, returned string can be just empty so if 'value' parameter conversion failed, then xbmc_envset can delete variable instead of setting new value for it.

_WIN32 is replaced.
And of course, it make sense to update only multiplatfrom code. Pure Linux/OSX code should use their native functions.

Member

jmarshallnz commented Aug 10, 2012

Before we even get into the code, where is the bug?

Member

Karlson2k commented Aug 11, 2012

At least one bug here, in Application.cpp:

CUtil::GetHomePath(xbmcPath);
SetEnvironmentVariable("XBMC_HOME", xbmcPath.c_str());

We are trying to set "XBMC_HOME", so next call to CUtil::GetHomePath should use this environment variable instead of full detection. But "GetHomePath" uses runtime function "_wgetenv" (which one operates with separate environment copy) and never get this variable, so every time "GetHomePath" uses full detection again.
I'm sure that in XBMC a lot of places where different environment is used. For example first setting "OS" variable will be ignored by any external libs, like python as it compiled by VS2008.

As I see, XBMC code uses widely "SetEnvironmentVariable" (as it simpler than "putenv") for setting variables, but uses "getenv" for reading it (as it simpler than "GetEnvironmentVariable"). But it's wrong, as "getenv" will not see changes made by "SetEnvironmentVariable" and vice versa.

To properly change variable on win32 platform you need to update it in:

  • runtime environment (by "putenv")
  • process environment (by "SetEnvironmentVariable", this environment is inherited by child processes )
  • other runtime versions environment used by third-party libs (each runtime uses independ copy of environment)

Currently XBMC uses in some places "win32env.c" to update environment for python, but "win32env.c" is buggy and could crash after library unloading.

If this code go to XBMC, we will need to get rid of "win32env.c".

Member

jmarshallnz commented Aug 11, 2012

Given that win32env.cpp comes directly out of PostgreSQL, I'd suggest if there are any bugs in that code, you should direct them to PostgreSQL first and foremost. Perhaps any bugs are really only due to it's use in XBMC?

If you want this accepted, remove all MultiByteToWideChar stuff, get rid of everything except char* (utf8) versions of put/get/set/unset designed so they work exactly like posix where doable.

Once the trees are cleared we might be able to see the forest.

Member

Karlson2k commented Aug 12, 2012

@jmarshallnz May be PostgreSQL don't unload libs at runtime - I don't know.
I just want to implement convenient functions for XBMC with XBMC's specific.

I removed all win32 charset conversions and split code to several commits.
4c5bd47 contains main logic, but main logic includes some CStdStringA and CStdStringW functions. The main reasons for it to explicitly return a copy of variable value (to not confuse with posix behavor as posix specify return value of getenv as modifiable, but this not possible on Windows platform with wide<->UTF-8 conversions). And CStdString is used for putenv (again - posix specify that string should be putted as later modifiable by it pointer and again it's not possible on windows).
CStdStringW is a part of main logic as some parts of XBMC use wide chars, so Environment can be modified without unnecessary wide->UTF8->wide conversions.

convertFromUtf8 and convertToUtf8 is just a wrappers for charsetconverter with additional checks and shortcuts useful mainly to Environment functions.

Member

Karlson2k commented Aug 26, 2012

@jmarshallnz @wsoltys
PR is still waiting.
Just check e777e57 at first.
This could solve number of problems and potential problems.
I'm still not sure about charsetconverter as SetEnv can be used before it initialization. May be implement win32 charset conversion at least as fallback?

Member

jmarshallnz commented Aug 26, 2012

Please drop the wide char versions - they're not required at all except for
internal to win32.

Further, you can probably drop the char * ones from the API as well and
just use const std::string &.

On Mon, Aug 27, 2012 at 10:08 AM, Karlson2k notifications@github.comwrote:

@jmarshallnz https://github.com/jmarshallnz @wsoltyshttps://github.com/wsoltys
PR is still waiting.
Just check e777e57 e777e57 at first.
This could solve number of problems and potential problems.
I'm still not sure about charsetconverter as SetEnv can be used before it
initialization. May be implement win32 charset conversion at least as
fallback?


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/1272#issuecomment-8038346.

Member

Karlson2k commented Aug 26, 2012

@jmarshallnz OK, wide char is really rare.
About "char *" - I just want to avoid extra char -> CStdString -> char conversion.
And the most common usage for setenv is "setenv("envvar", varvalue)", so if we accept "char *" we'll just get a pointer without any initialization/conversion/c_str.
I'll start with dropping of wide char.

Member

jmarshallnz commented Aug 26, 2012

I know, thus the suggest to accept a const std::string & which will quite happily take both.

Member

Karlson2k commented Aug 29, 2012

@jmarshallnz Rewritten.
Now uses mostly std::string and std::wstring.

Member

Karlson2k commented Oct 9, 2012

@jmarshallnz What can I do to improve this PR even more? :)

Member

Karlson2k commented Apr 6, 2013

@jmarshallnz @wsoltys
Is it time?

Member

jmarshallnz commented Apr 7, 2013

XBMC does everything in utf8. Drop all wstring functions as they're not needed.

Under which circumstances is the charset convertor not available?

Member

wsoltys commented Apr 7, 2013

Apparently a wchar version was needed: c5f50fb

Member

jmarshallnz commented Apr 7, 2013

Isn't that one just being used for utf8?

On Mon, Apr 8, 2013 at 9:15 AM, wsoltys notifications@github.com wrote:

Apparently a wchar version was needed: c5f50fbhttps://github.com/xbmc/xbmc/commit/c5f50fb41e4a8353a1817ffcd127a1706b838ccf


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/1272#issuecomment-16024384
.

Member

wsoltys commented Apr 8, 2013

@chadoe can you tell us why the UTF8->wchar conversion was needed?

Member

chadoe commented Apr 8, 2013

I didn't read everything here but for the change @wsoltys is referring to:
Because there's the ansi and the wide putenv api function... and special character paths stored in utf8 converted to ansi could result in loss of characters with certain characters that users use in their windows username. Which python would then reading back from the environment (invalid paths).
But maybe some of the many later fixes makes using the wide api obsolete, I remember making use of system codepages for converting the paths later on...

Member

Karlson2k commented Apr 8, 2013

@wsoltys @jmarshallnz wchar version is mostly for win32-only code sections. It's easy to use wchar in win32 as windows doesn't support UTF-8 as input/output (except charset conversion functions).

@jmarshallnz XBMC's charset converter is not available before initialization / if shutdown in progress. Same as for CLog: https://github.com/xbmc/xbmc/blob/master/xbmc/utils/log.cpp#L226

Member

jmarshallnz commented Apr 8, 2013

Is the environment stuff even used before initialization or after shutdown? If it is, then I'd tend to use the win32 API entirely for the utf8->wide conversion. If it's not, use the charsetconverter everywhere. Mixing and matching doesn't make much sense.

XBMC doesn't use a wchar version of set/putenv at all so requires only a utf8 version (obviously wrapped to the wide API). There's no point having wrapping code that isn't used.

Member

Karlson2k commented Apr 8, 2013

Wchar is used only in https://github.com/xbmc/xbmc/blob/master/xbmc/win32/WIN32Util.cpp#L503 and https://github.com/xbmc/xbmc/blob/master/xbmc/win32/WIN32Util.cpp#L513, so wrappers can be really removed.
And I'll check initialization/deinitialization order, but it can be changed in future (or will require environment manipulation in different moment). As utf8<->wchar conversion needed only for win32 platform, it will be more future proof to use win32 conversion (and no conversion for other platforms).

Should I move all function to some class (XBMCEnv?) or to another namespace?

Member

jmarshallnz commented Apr 8, 2013

Yeah, agreed - just use the win32 API for conversion to keep it simple.

It might be useful to wrap in a (static?) class I guess. If so, keep the name of the functions simple, corresponding to the name of the posix function (SetEnv, PutEnv etc.)

Member

Karlson2k commented Apr 9, 2013

@jmarshallnz Rebased, all public wchar_t/wstring functions removed, WStdString usage removed, everything moved into static class, some functions was refactored, some rewritten.
Makefile updated. Need help on updating Mac projects.

xbmc/utils/XbmcEnv.cpp
+ return CXbmcEnv::win32ConvertWToUtf8(Wvalue.c_str());
+#else
+ return std::string(::getenv(name.c_str()));
+#endif
@jmarshallnz

jmarshallnz Apr 9, 2013

Member

do you actually need the std::string() wrapper?

@Karlson2k

Karlson2k Apr 9, 2013

Member

Not really. It'll be used by compiler in any case.
I use it for clarity of transforming result type.
If you say that readability is better without it, I'll remove it.

@jmarshallnz

jmarshallnz Apr 9, 2013

Member

IMO it's better without.

xbmc/utils/XbmcEnv.cpp
+ Wvalue = valBuf;
+ delete[] valBuf;
+
+ return CXbmcEnv::win32ConvertWToUtf8(Wvalue.c_str());
@jmarshallnz

jmarshallnz Apr 9, 2013

Member

No need to prefix with CXbmcEnv

@Karlson2k

Karlson2k Apr 9, 2013

Member

That's for consistency with CXbmcEnv::setenv (and other member function).
And again, for clarity - used local helper function.
But I will not insist. Remove?

@jmarshallnz

jmarshallnz Apr 9, 2013

Member

Remove them all. We already know which class we're in, there's no need to tell the compiler. The :: on the functions that are global make it clear they're global.

xbmc/utils/XbmcEnv.cpp
+ if (pos == 0) // '=' is the first character
+ return -1;
+ if (pos == std::string::npos)
+ return CXbmcEnv::unsetenv(envstring);
@jmarshallnz

jmarshallnz Apr 9, 2013

Member

No need to prefix with CXbmcEnv

xbmc/utils/XbmcEnv.cpp
+ */
+
+/*
+ * Copyright (C) 2012 Team XBMC
@jmarshallnz

jmarshallnz Apr 9, 2013

Member

Update copyright

xbmc/utils/XbmcEnv.cpp
+ *
+ * Some ideas was inspired by 'src/port/win32env.c' file.
+ * Refined, updated, enhanced and modified for XBMC by Karlson2k.
+ */
@jmarshallnz

jmarshallnz Apr 9, 2013

Member

Move to underneath copyright message.

xbmc/utils/XbmcEnv.cpp
+ if (resultSuccessful != NULL)
+ *resultSuccessful = true;
+ return Wret;
+}
@jmarshallnz

jmarshallnz Apr 9, 2013

Member

This functions might work better as:

bool win32ConvertUtf8ToW(const char *text, std::wstring &result)

as you tend to check the return code rather than the resulting string?

The next one you tend to use unchecked (as that makes most sense) so it doesn't matter so much, but they should probably be consistent.

@Karlson2k

Karlson2k Apr 9, 2013

Member

Need to check result only in one or two places (to avoid accidental variable deletion instead of modifying). In most cases only string result is needed.
But in that form I loose ability to easy use it in form like

return win32ConvertWToUtf8(somfunc(win32ConvertUtf8ToW(str)));

With bool return this nice code is transformed into

std::wstring wstr;
win32ConvertUtf8ToW(str, wstr);
std::string strRet;
win32ConvertWToUtf8(somefunc(wstr), strRet);
return strRet;

I could replace pointer to bool with reference to bool. Than function can use it without check for NULL.

@jmarshallnz

jmarshallnz Apr 9, 2013

Member

You use the resulting string from WToUtf8 in this way once (the only time it's used, so you never check the return code boolean). You use the bool from Utf8ToW as a check 2 out of 3 times, and the 3rd time you do a check based on the emptiness of the returned string (which is OK in that case).

It doesn't really matter either way - just something I noticed that looked a bit ugly. :)

The problem with reference to bool is you can't ignore it completely (when you don't care about errors) without having an ugly wrapper.

project/VS2010Express/XBMC.vcxproj.filters
@@ -5877,4 +5883,4 @@
<Filter>interfaces\swig</Filter>
</None>
</ItemGroup>
-</Project>
+</Project>
@MartijnKaijser

MartijnKaijser Apr 9, 2013

Owner

This line end won't cause issues? Same in other project file.

@Karlson2k

Karlson2k Apr 9, 2013

Member

No issues. It's modified by VS and XML is loaded correct in any case.
But I'll add linebreak.

xbmc/utils/XbmcEnv.cpp
+ return Wret;
+}
+
+std::string CXbmcEnv::win32ConvertWToUtf8(const wchar_t *text, bool *resultSuccessful /*= NULL*/)
@jmarshallnz

jmarshallnz Apr 9, 2013

Member

If you just use const wstring &text you don't have to check for NULL, and also don't need to .c_str() the input.

xbmc/utils/XbmcEnv.h
+ * All functions work on all platforms. Special care is taken on Windows platform where Environment is changed for process itself,
+ * in own runtime library and in all runtime libraries (MSVCRT) loaded by third-party modules.
+ * Functions internally make all necessary UTF-8 <-> wide conversions.
+ */
@jmarshallnz

jmarshallnz Apr 9, 2013

Member

Put this as the doxy on the class.

xbmc/utils/XbmcEnv.h
+ * 2. CXbmcEnv::setenv parameter 'overwrite' is optional, set by default to 1 (allow overwrite)
+ * 3. CXbmcEnv::putenv uses copy of provided string (rather than string itself) to change environment.
+ * 4. CXbmcEnv::putenv can be used to unset variables. Set parameter to 'var=' (Windows style) or just 'var' (POSIX style), and 'var' will be unset.
+ * 5. CXbmcEnv::getenv returns a copy of environment variable value instead of pointer to value.
@jmarshallnz

jmarshallnz Apr 9, 2013

Member

Which of these exceptions do we make use of, other than the copies rather than pointers to value? Perhaps it makes sense to just have the copies instead of the behaviour changes? I dunno :)

@Karlson2k

Karlson2k Apr 9, 2013

Member
  1. setenv with empty string is consistent with MS CRT method of deletion of variables. And doesn't contradict with POSIX.
  2. making 'overwrite' optional is simple convenient in C++, and in most cases overwrite is set to '1'.
  3. putenv use std::string, so it just clarification
  4. it's MS-style
  5. by POSIX environment can be changed using returned pointer. As std::string used, it's just clarification.

Actually, all point are more extending, rather than exceptions.
If it looks too complex, this notice can be removed, as functions can be used in POSIX-style.

@jmarshallnz

jmarshallnz Apr 9, 2013

Member

Maybe just add a note to that effect? i.e. "You can generally use the functions as you would normally in POSIX-style. The differences below are just to make things more convenient through use of std::string (2,3,5), and to also allow the Win32 method of unsetting variables (1+4) if wanted."

@Karlson2k

Karlson2k Apr 9, 2013

Member

Yes, this is definitely better and simpler. :)

xbmc/utils/XbmcEnv.h
+ autoDetect = 1,
+ };
+ static int win32_setenv(const std::string &name, const std::string &value = "", updateAction action = autoDetect);
+#endif // TARGET_WINDOWS
@jmarshallnz

jmarshallnz Apr 9, 2013

Member

These can probably just go in the .cpp

Member

jmarshallnz commented Apr 9, 2013

Just a few cosmetic comments more than anything. One last one: I'd name it Environment.h/cpp (CEnvironment) - we don't need to know it's in Xbmc :)

Member

Karlson2k commented Apr 11, 2013

@jmarshallnz Done + few more cosmetics.

xbmc/Application.cpp
@@ -344,6 +344,10 @@
#include "android/activity/XBMCApp.h"
#endif
+#ifdef TARGET_WINDOWS
+#include "utils/XbmcEnv.h"
@jmarshallnz

jmarshallnz Apr 14, 2013

Member

Environment.h

@Karlson2k

Karlson2k Apr 15, 2013

Member

Thanks! Fixed.

@ghost ghost assigned jmarshallnz Apr 14, 2013

Member

jmarshallnz commented Apr 15, 2013

Looks good. Build needs testing on Linux, else drop the Makefile change. Will merge in May window.

Member

Karlson2k commented Apr 17, 2013

Small correction for Linux (replace "pop_back" with "erase" for std::string). "pop_back" is part of C++11, but for it's available on MSVC C++98.
Tested build on Ubuntu v12.10 x32.

Member

Karlson2k commented May 7, 2013

@jmarshallnz Is it OK for May window?

Member

Karlson2k commented May 7, 2013

@Memphiz Needs sync xcode projects.

Member

Karlson2k commented May 7, 2013

Fixed typo in comment

Member

Karlson2k commented May 10, 2013

@jmarshallnz Postpone until next window?

Member

jmarshallnz commented May 10, 2013

If you drop the Makefile change it shouldn't affect other platforms at all, right?

Or, if you've build tested on linux, the Makefile change can stay.

Either or, I'm happy to pull this in as soon as that is done (this window).

Member

Karlson2k commented May 11, 2013

It's build tested on Linux (Ubuntu).
I think, it's fully ready to merge. :)

Member

jmarshallnz commented May 11, 2013

Not according to github - needs rebase.

Member

Karlson2k commented May 11, 2013

@jmarshallnz, it's ready again. :)

jmarshallnz added a commit that referenced this pull request May 11, 2013

Merge pull request #1272 from Karlson2k/xbmc_setenv_clean
Implementation of platform-independ environment manipulations

@jmarshallnz jmarshallnz merged commit 1fb487a into xbmc:master May 11, 2013

Member

jmarshallnz commented May 11, 2013

Thanks :)

Member

Karlson2k commented May 12, 2013

@Memphiz Branch was merged, need xcode projects sync.

Owner

Memphiz commented May 13, 2013

@Karlson2k - done - i guess it needs codechanges somewhere to make use of it?

Member

Karlson2k commented May 13, 2013

Those functions are mostly wrappers around system native function. Special tricks used only on win32.
So for non-win32 you can continue use system native functions in system specific code or use this class if it's more convenient.
No need to convert old system specific code.
Those functions must be used for win32 code and common code parts (for win32 and non-win32).
It's done in #2739.
@Memphiz, could your help on checking PR #2739 for ios/osx?

"unsigned int" in order to get rid of the sign-compare warnings in line 240 and 242.

Member

Karlson2k replied May 21, 2013

Thanks!
Here: 9d68380 in #2739

LongChair added a commit to plexinc/plex-home-theater-public that referenced this pull request Aug 1, 2014

Fix Error "The reponse from the server did not make sense" #1272.
We shouldn't display that error message to the GUI. It's been replaced by an error log message as requested.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment