Created attachment 105816 [details] [review] proposed D-Bus patch I found two issues in D-Bus 1.8.0 which prevent it from being properly run (and run services). a) The _dbus_replace_install_prefix() damages Exec dir when its prefix doesn't match the build prefix. There was a typo, user 'strcat' instead of 'srcpy'. As there was also missing replace of backslashes to forward slashes, then I changed that as well. b) User's home path doesn't always match HOMEDRIVE + HOMEPATH, thus I took the code from GLib and adapted it to D-Bus, thus the GLib can find D-Bus cookies (when D-Bus saves them to the correct folder). As an example, msys sets the two variables as: "C:" and "\", which means that D-Bus was saving data into the root folder, where it might not have always access.
Comment on attachment 105816 [details] [review] proposed D-Bus patch Review of attachment 105816 [details] [review]: ----------------------------------------------------------------- ::: dbus-1.8.0.old/dbus/dbus-sysdeps-util-win.c @@ +1557,5 @@ > strlen (DBUS_PREFIX) + 1))) { > + strcpy (retval, configure_time_path); > + } else { > + strcpy (retval, runtime_prefix); > + strcat (retval, configure_time_path + strlen (DBUS_PREFIX) + 1); Not a regression, but this path manipulation really ought to be using DBusString. ::: dbus-1.8.0.old/dbus/dbus-sysdeps-win.c @@ +3393,5 @@ > + if (hr == S_OK) > + { > + b = SHGetPathFromIDListA (pidl, path); > + if (b) > + retval = strdup (path); Better to use _dbus_strdup() or DBusString, I think @@ +3425,5 @@ > + break; > + } > + } > + > + strcat(windowsdir, sep); Non-length-checked string concatenation is Very Bad. We have DBusString for a reason; please use it. @@ +3455,5 @@ > + /* In case HOME is Unix-style (it happens), convert it to > + * Windows style. > + */ > + char *p; > + tmp = strdup(env); _dbus_strdup() (paired with dbus_free()) or DBusString would probably be better @@ +3482,5 @@ > + if (!tmp) > + tmp = get_special_folder(CSIDL_PROFILE); > + > + if (!tmp) > + tmp = get_windows_directory_root(); Does it ever make sense to use C:\Windows (or whatever) as the "home directory"?
review- for use of strcat(). I don't know much about Windows conventions so I would appreciate someone who does (Ralf?) checking whether the semantic changes here make sense.
(In reply to comment #0) > thus I took the code from GLib and adapted it to D-Bus Sorry, libdbus is under a silly license (GPL-2+ | AFL-2.1) which we can no longer change because one of the copyright holders has gone out of business, so we can't accept code under LGPL (that would change the effective license of libdbus to GPL, which is undesirable). Can you ask any relevant copyright holders to license this stuff permissively, or summarize the logic that GLib uses so someone can reimplement it under a permissive license?
I guess Matthias can answer your questions. With respect of the patch itself, as you quoted at comment #3, I just took what GLib uses and copied the code to D-Bus, thus they operate on the same directories. I'm sorry, but I do not know whether I'll be able to do more thorough rewrite of the patch, the DBusString is too new to me. Nonetheless, I agree that it makes sense to test the code, not write it blindly, thus in case you'd like to test/finish something rewritten, then I can definitely do it.
I have very little secret knowledge about the win32 specific code in glib - you can read the code as well as I can.
(In reply to comment #5) > I have very little secret knowledge about the win32 specific code in glib - > you can read the code as well as I can. Matthias, the more important question was about licensing and copying the code from GLib to D-Bus, at comment #3. Could you answer that, or point here someone from the GLib team whom might know the answer, please?
you have to look at who wrote the code. git can give a much more accurate answer about that than me.
I asked git and it showed that the main author of the copied functions get_special_folder() and get_windows_directory_root() and of the significant part of the body of _dbus_set_user_home() is Tor, thus I added him to the CC. Tor, would you mind, if your code would be copied from GLib into D-Bus code base, please? See comment #3 with a mentioned licensing issue.
(In reply to comment #0) > a) The _dbus_replace_install_prefix() damages Exec dir when its prefix > doesn't match the build prefix. There was a typo, user 'strcat' instead of > 'srcpy'. As there was also missing replace of backslashes to forward > slashes, then I changed that as well. Please describe a situation in which it fails to work (e.g. "when I built dbus with --prefix=C:\foo and installed into C:\bar, I expected it to look for [what?] in [where?] but actually it looked in [where?]") so reviewers know what they're looking at. > b) User's home path doesn't always match HOMEDRIVE + HOMEPATH [...] > As an example, msys > sets the two variables as: "C:" and "\", which means that D-Bus was saving > data into the root folder, where it might not have always access. Isn't that a bug in msys? Is there some specification, however informal, for how interoperating tools that originated on Unix should behave on Windows? ("they should do exactly what GLib does" would be better than nothing)
(In reply to comment #8) > Tor, would you mind, if your code would be copied from GLib into D-Bus code > base, please? See comment #3 with a mentioned licensing issue. What we would need is permission from the copyright holder to license that code under either: - the dual-license used by libdbus (GPL-2+ or AFL-2.1, at licensee's option) or preferably, - a permissive license compatible with both branches of that dual-license (ideally the version of the MIT/X11 license used by Expat, which appears in e.g. test/dbus-daemon.c)
I assume I did that stuff while working for Novell, the relevant parts of which is nowadays then known as SUSE, so in case they have some generic statement for this kind of things, just rely on that, otherwise you will have to find somebody there to ask...
(If it was just me personally, I would gladly agree to any kind of open source re-licensing.)
(In reply to comment #12) > (If it was just me personally, I would gladly agree to any kind of open > source re-licensing.) git blame showed you as the author of the copied code, nobody else :)
(In reply to comment #9) > (In reply to comment #0) > > a) The _dbus_replace_install_prefix() damages Exec dir when its prefix > > doesn't match the build prefix. There was a typo, user 'strcat' instead of > > 'srcpy'. As there was also missing replace of backslashes to forward > > slashes, then I changed that as well. > > Please describe a situation in which it fails to work (e.g. "when I built > dbus with --prefix=C:\foo and installed into C:\bar, I expected it to look > for [what?] in [where?] but actually it looked in [where?]") so reviewers > know what they're looking at. I made a typo, not "user 'strcat'", but "uses 'strcat'". The D-Bus code uses strcat on a static variable, thus when you have multiple services, then the Exec paths are concatenating instead of being used verbatim (as 'strcpy' does). That means that the first call returns "C:\bar\bin\service1", the second call returns "C:\bar\bin\service1C:\bar\bin\service2". You can get even out of bounds of the static string easily, with enough services installed. > > b) User's home path doesn't always match HOMEDRIVE + HOMEPATH [...] > > As an example, msys > > sets the two variables as: "C:" and "\", which means that D-Bus was saving > > data into the root folder, where it might not have always access. > > Isn't that a bug in msys? I do not know, maybe it is. I agree that cmd.exe has these paths set to user's home. > Is there some specification, however informal, for how interoperating tools > that originated on Unix should behave on Windows? ("they should do exactly > what GLib does" would be better than nothing) I cannot answer this, I'm not aware of anything. It just seemed logic to use better options than environment variables.
> git blame showed you as the author of the copied code, nobody else Sure, but as I probably did the work while being employed by Novell, as part of my job, it is they (or their legal successors) who can claim copyright to the code.
I see. To make this simpler, if I rewrite the code, which will follow the logic you wrote, but the code will be different, like when using D-Bus strings instead of plain char arrays, will it be any difference from the license point of view? Being a nitpick, I can find similar code in MSDN's (MS IDE) examples too, there is nothing special about it as such (please see the proposed patch).
Well, as I am sure you know, I am not a lawyer, and can not give legal advice;) But indeed, I agree the code is fairly trivial.
Created attachment 106930 [details] [review] Exec fix Let's split this into two patches. This is the first part, which fixes the Exec path creation for services (more services, doing strcat() on a static string leads to wrong path for the second+ service, not talking about missed corrections of the path in this if-branch).
Created attachment 106931 [details] [review] get path for cookies This is about the bits from GLib. I rewrote it: a) using D-Bus String b) keeping only ideas from GLib, not the exact code (though left there the comments)
Created attachment 113363 [details] [review] Add manual-paths test executable This patch adds a manual paths helper executable to be able to perform a manual check of _dbus_replace_install_prefix()
Created attachment 113364 [details] [review] Add manual-paths test executable Added a check for non DBUS_PREFIX related pathes.
Comment on attachment 113364 [details] [review] Add manual-paths test executable Review of attachment 113364 [details] [review]: ----------------------------------------------------------------- Some coding style nitpicking, but looks fine to merge if it'll be useful to you. ::: cmake/test/CMakeLists.txt @@ +74,5 @@ > add_helper_executable(test-sleep-forever ${test-sleep-forever_SOURCES} ${DBUS_INTERNAL_LIBRARIES}) > add_test_executable(manual-tcp ${manual-tcp_SOURCES} ${DBUS_INTERNAL_LIBRARIES}) > +if(WIN32) > + add_helper_executable(manual-paths ${manual-paths_SOURCES} ${DBUS_INTERNAL_LIBRARIES}) > +endif() I would like to be able to build this with mingw/Autotools, but I can add that later if you'll review it. ::: test/manual-paths.c @@ +21,5 @@ > + > + if (!_dbus_get_install_root(runtime_prefix, sizeof(runtime_prefix))) > + { > + fprintf(stderr, "dbus_get_install_root() failed\n"); > + return FALSE; Something has gone a bit wrong with the indentation, particularly in this function... @@ +34,5 @@ > + DBusList *link; > + dirs = NULL; > + > + if (!_dbus_get_standard_session_servicedirs (&dirs)) > + _dbus_assert_not_reached ("couldn't get stardard dirs"); standard @@ +59,5 @@ > + > +int > +main (int argc, char **argv) > +{ > + if (print_install_root() == FALSE) I'd prefer if (!print_install_root ()), etc.
Comment on attachment 106931 [details] [review] get path for cookies Review of attachment 106931 [details] [review]: ----------------------------------------------------------------- I cannot comment on the correctness of this code's interactions with Windows beyond "it doesn't look obviously wrong", only on its interactions with the rest of libdbus. ::: dbus-1.8.0.old/dbus/dbus-sysdeps-win.c @@ +3404,5 @@ > + len = strlen (env); > + if (len < MAX_PATH) > + { > + char *p; > + strncpy(path, env, len); The point of DBusString is that it's an automatically-resized string buffer, like GLib's GString or C++'s std::string, so that you don't have to deal with either buffer overflows, or arbitrary length limits like MAX_PATH. The right pseudocode would be something like this: if (!_dbus_string_append (homedir, env)) return FALSE; /* OOM */ for (i = 0; i < _dbus_string_get_length (homedir); i++) if (_dbus_string_get_byte (homedir, i) == '/') _dbus_string_set_byte (homedir, i, '\\'); No MAX_PATH, no strncpy, and no buffer overflows. @@ +3411,5 @@ > + { > + *p = '\\'; > + } > + > + if ((path[0] == '\\' || (((path[0] >= 'a' && path[0] <= 'z') || (path[0] >= 'A' && path[0] <= 'Z')) && path[1] == ':')) && You could use const char *path; ... path = _dbus_string_get_const_data (homedir); to do this bit, and the GetFileAttributesA calls. @@ +3415,5 @@ > + if ((path[0] == '\\' || (((path[0] >= 'a' && path[0] <= 'z') || (path[0] >= 'A' && path[0] <= 'Z')) && path[1] == ':')) && > + GetFileAttributesA (path) != INVALID_FILE_ATTRIBUTES && > + (GetFileAttributesA (path) & FILE_ATTRIBUTE_DIRECTORY) != 0) > + { > + _dbus_string_append (homedir, path); _dbus_string_append() can return FALSE on OOM. The right response is to free any local things (there are none here, as far as I can see) and return FALSE. @@ +3441,5 @@ > + { > + _dbus_string_append (homedir, path); > + } > + > + return _dbus_string_get_length (homedir) != 0; Functions in libdbus are expected to deal with OOM, unlike GLib. The conventional API here would probably be: * if OOM: return FALSE * if home directory could not be found: return TRUE with homedir having zero length * if home directory was found: return TRUE with homedir having nonzero length @@ +3445,5 @@ > + return _dbus_string_get_length (homedir) != 0; > +} > + > +static void > +_dbus_set_homedir_from_drive_and_path_env(DBusString *homedir) Similarly, this needs to return a boolean for whether there was enough memory. @@ +3495,2 @@ > { > + _dbus_set_homedir_from_drive_and_path_env(&homedir); With OOM handling, this would have to be something like this pseudocode: if (!_dbus_set_user_home(&homedir)) return FALSE; if (homedir has length 0) { if (!_dbus_set_homedir_from_drive_and_path_env(&homedir)) return FALSE; } if (homedir has length 0) { /* Should not happen, but just in case */ if (cannot append "C:\\") return FALSE; }
Comment on attachment 106930 [details] [review] Exec fix Review of attachment 106930 [details] [review]: ----------------------------------------------------------------- ::: dbus-1.8.0.old/dbus/dbus-sysdeps-util-win.c @@ +1557,5 @@ > strlen (DBUS_PREFIX) + 1))) { > + strcpy (retval, configure_time_path); > + } else { > + strcpy (retval, runtime_prefix); > + strcat (retval, configure_time_path + strlen (DBUS_PREFIX) + 1); I would really prefer DBusString here, but failing that, it should at least be strnwhatever rather than strwhatever so that it can't overflow the buffer.
Created attachment 113377 [details] [review] Add manual-paths test executable (update) - fix indention - fix spelling (copy and paste bug from dbus/config-parser.c) - fix "if (!print.."
(In reply to Simon McVittie from comment #24) > Comment on attachment 106930 [details] [review] [review] > Exec fix > > Review of attachment 106930 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus-1.8.0.old/dbus/dbus-sysdeps-util-win.c > @@ +1557,5 @@ > > strlen (DBUS_PREFIX) + 1))) { > > + strcpy (retval, configure_time_path); > > + } else { > > + strcpy (retval, runtime_prefix); > > + strcat (retval, configure_time_path + strlen (DBUS_PREFIX) + 1); > > I would really prefer DBusString here, but failing that, it should at least > be strnwhatever rather than strwhatever so that it can't overflow the buffer. Should _dbus_replace_install_prefix () be thread safe ? There are static buffers inside ... static char retval[1000]; static char runtime_prefix[1000];
(In reply to Ralf Habacker from comment #26) > Should _dbus_replace_install_prefix () be thread safe ? There are static > buffers inside > ... > static char retval[1000]; > static char runtime_prefix[1000]; If it's in foo-util.c it does not need to be thread-safe. If it's compiled into libdbus-1, it should be thread-safe. According to my past self on Bug #68610, this isn't in libdbus-1 any more (but DBusString would still be better than a static buffer).
(In reply to Ralf Habacker from comment #25) > - fix spelling (copy and paste bug from dbus/config-parser.c) should be better "spelling bug is in config-parser.c 3 times, which has been copy and pasted"
Comment on attachment 113377 [details] [review] Add manual-paths test executable (update) Review of attachment 113377 [details] [review]: ----------------------------------------------------------------- Go ahead
Comment on attachment 113377 [details] [review] Add manual-paths test executable (update) committed to master
(In reply to Simon McVittie from comment #22) > I would like to be able to build this with mingw/Autotools, but I can add > that later if you'll review it. It's trivial, so I just pushed it. Here's the commit in case anyone objects: http://cgit.freedesktop.org/dbus/dbus/commit/?id=890b1dd5c5015203fb0e5235d06528b362879bd0
Comment on attachment 106930 [details] [review] Exec fix Review of attachment 106930 [details] [review]: ----------------------------------------------------------------- Looks like nobody ever fixed this. My previous review comments still stand: for the development branch it should really be DBusString, and for stable-branches it should at least be strncpy/strncat instead of strcpy/strcat so it won't overflow the buffer.
Created attachment 118346 [details] [review] Fix creation of Exec path for files not in prefix From: Milan Crha <mcrha redhat com> Doing strcat() into a static buffer produces incorrect results for the second and subsequent services if they are not in the ${prefix}; for example, if the first call should have returned "C:\bar\bin\service1" and the second should have returned "C:\bar\bin\service2", the second result would actually be "C:\bar\bin\service1C:\bar\bin\service2". [smcv: added commit message; used strncpy/strncat to avoid overflow] --- Briefly tested with manual-paths.exe and Wine; not stress-tested. I'm not sure why I've ended up writing patches for an OS I don't even use.
Created attachment 118349 [details] [review] Rename getters for session, system config files It turns out to be easier to implement the Windows version of these in a relocatable way if it can assume that the argument starts empty, which is in fact true in practice.
Created attachment 118350 [details] [untested] Use DBusString for all relocation and install-root code --- For the development branch only. This requires Attachment #118346 [details], Attachment #118349 [details] and the patches from Bug #92028. It compiles, but I haven't tested it. The test case test_default_session_servicedirs() may need some adjustment: it seems to be doing wrong things (appending DBUS_DATADIR to the install root, whereas it should only be appending the part of DBUS_DATADIR that is not ${prefix}). This is a "technical debt" fix: the general approach to string manipulation should always have been like this.
Comment on attachment 118349 [details] [review] Rename getters for session, system config files Review of attachment 118349 [details] [review]: ----------------------------------------------------------------- looks good
(In reply to Ralf Habacker from comment #36) > looks good Thanks. How about Attachment #118346 [details] (for 1.10 if you want runtime path discovery to work there, or master), Attachment #118347 [details] from Bug #92028 (for master), Attachment #118350 [details] (for master)? In particular please check my buffer-sizing arithmetic in Attachment #118346 [details] - I might have got it wrong. Attachment #118350 [details] avoids that entirely, but unfortunately I think the platform-independent parts of that change are too intrusive to be appropriate for a stable-branch. Test results also very welcome, particularly from real Windows systems (i.e. not Wine).
Comment on attachment 118346 [details] [review] Fix creation of Exec path for files not in prefix Review of attachment 118346 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-util-win.c @@ +1503,5 @@ > + retval[sizeof (retval) - 1] = '\0'; > + remaining = sizeof (retval) - 1 - strlen (retval); > + strncat (retval, > + configure_time_path + strlen (DBUS_PREFIX) + 1, > + remaining); looks good.
Comment on attachment 118346 [details] [review] Fix creation of Exec path for files not in prefix Applied. Fixed in git for 1.10.2 and 1.11.0 (but I'm leaving this bug open, because we still have the technical debt of "this should always have been a DBusString").
Comment on attachment 118349 [details] [review] Rename getters for session, system config files Applied for 1.11.0
Comment on attachment 106931 [details] [review] get path for cookies Review of attachment 106931 [details] [review]: ----------------------------------------------------------------- I already reviewed this and suggested changes.
Created attachment 118540 [details] [review] [untested] Use DBusString for all relocation and install-root code This means we handle OOM correctly, and makes it obvious that we are not overflowing buffers. This change does not affect the actual content of the strings. Instead of redefining DBUS_DATADIR to be a function call (which hides the fact that DBUS_DATADIR is used), this patch makes each use explicit: DBUS_DATADIR is always the #define from configure or cmake, before replacing the prefix. --- This includes and obsoletes Attachment #118347 [details]. It needs review, and testing from someone who actually uses Windows.
Comment on attachment 118540 [details] [review] [untested] Use DBusString for all relocation and install-root code Review of attachment 118540 [details] [review]: ----------------------------------------------------------------- Looks good except the two mentioned issues. While reviewing I got the impression that having a dbus internal C++ variant of DBusString would reduce the required numbers of dbus_string_free() calls very much, because the compiler would call the DBusString destructor for cleanup by default on leaving the block in which the related instance has been created. ::: bus/config-parser.c @@ +3416,2 @@ > { > + if (!_dbus_string_append (&install_root_based, DBUS_DATADIR) || Adding DBUS_DATADIR, which is expanded to an absolute configure time path, to the runtime install root will result into an invalid path. ::: dbus/dbus-sysdeps-win.c @@ +3345,1 @@ > lastSlash = _mbsrchr(prefix, '\\'); Not your change, but the path is returned from the non multi byte version of GetModuleFileName. It looks wrong to use this multi byte function to search for the last slash.
(In reply to Ralf Habacker from comment #43) > While reviewing I got the impression that having a dbus internal C++ variant > of DBusString would reduce the required numbers of dbus_string_free() calls It's an interesting idea, but no, I'm not going to make libdbus C++. > Adding DBUS_DATADIR, which is expanded to an absolute configure time path, > to the runtime install root will result into an invalid path. I don't think this code is correct either, which is why I said "does not affect the actual content" in the commit message :-) As far as I can see, the old code (with error handling omitted) was: _dbus_get_install_root(buffer, sizeof(buffer)); strcat(buffer,DBUS_DATADIR); strcat(buffer,"/dbus-1/services"); so I'm just replicating that logic. If you believe the logic is wrong, I'd be happy to review a fix, but can we please make it buffer-overflow-safe first? Looking at the build systems, it seems this might actually be correct under CMake but not Autotools, because CMake defines DBUS_DATADIR to be just "share"... we should audit all the remaining uses of DBUS_DATADIR, which will be easier after this patch makes dbus-sysdeps-util-win.c stop redefining it. > Not your change, but the path is returned from the non multi byte version of > GetModuleFileName. It looks wrong to use this multi byte function to search > for the last slash. As above, I'm just replicating the current logic with more buffer-safety. If the logic is wrong, I'm happy to review a fix to that, but I'd prefer to land this first.
(In reply to Simon McVittie from comment #44) > we should audit all the remaining uses of DBUS_DATADIR, which > will be easier after this patch Assuming that patch is applied, we have: * Windows _dbus_get_standard_session_servicedirs() is fine either way; we can remove some complexity if we make DBUS_DATADIR always absolute (and then relocate it, which we already do), for instance by using "#define DBUS_DATADIR "@EXPANDED_DATADIR@" in config.h.cmake. * Windows test_default_session_servicedirs() basically assumes it's relative to the install root (which is what you just pointed out); it would be easy to relocate an absolute DBUS_DATADIR here, though * Unix _dbus_get_standard_session_servicedirs() basically assumes it's absolute, and might search relative to getcwd() if built with CMake * Unix _dbus_get_standard_system_servicedirs() likewise * Unix test_default_session_servicedirs() likewise * Unix test_default_system_servicedirs() likewise Under Autotools, we set DBUS_DATADIR=$EXPANDED_DATADIR anyway. So I think we should just redefine DBUS_DATADIR to be "always absolute, if you want to support relocation you must do it in C code".
(In reply to Simon McVittie from comment #45) > So I think we should just redefine DBUS_DATADIR to be "always absolute" ... which, actually, it is already: if (NOT DBUS_DATADIR) SET(DBUS_DATADIR ${DATADIR}) endif() ... set(EXPANDED_DATADIR ${DBUS_INSTALL_DIR}/${DBUS_DATADIR}) ... set(DBUS_DATADIR ${EXPANDED_DATADIR}) This redefinition is really confusing. Can we just drop DATAROOTDIR, DATADIR and DOCDIR from the CMake stuff?
(In reply to Ralf Habacker from comment #43) > Not your change, but the path is returned from the non multi byte version of > GetModuleFileName. It looks wrong to use this multi byte function to search > for the last slash. Are sure you aren't mixing up multi-byte strings with wide strings? GetModuleFileName() means either GetModuleFileNameA() (some ANSI code page) or GetModuleFileNameW() ("wide", i.e. UCS-2), and as far as I'm aware, we consistently use the ANSI versions, which return a string encoded according to the system code-page. If the system code-page is something multi-byte like Shift-JIS or UTF-8, isn't _mbsrchr() more correct than strrchr()?
Created attachment 118617 [details] [review] Assume that DBUS_DATADIR is absolute on Windows Both build systems arrange for this to be the case, and we already assume that it's absolute on Unix. On Windows, it's probably going to be /mingw/share or something; it gets relocated via _dbus_replace_install_prefix() at runtime. --- How about this?
Created attachment 118618 [details] [review] cmake: simplify definition of installation paths In particular, changing the meaning of DBUS_DATADIR part way through the file is really confusing. --- If we don't have a use for these variables, we should just not use them. "git grep" says we don't, although I suppose someone could conceivably be setting them with -D on the command line? Or we could keep DATAROOTDIR and DATADIR, but simplify it so DATADIR is relative, EXPANDED_DATADIR is set from DATADIR, and DBUS_DATADIR is absolute. DOCDIR doesn't seem to be used at all. On Autotools, which seems to be where the naming came from, the equivalent variables are ${datarootdir}, ${datadir} and ${docdir}, and their values normally contain unexpanded variable references: prefix=/usr datarootdir='${prefix}/share' datadir='${datarootdir}' docdir='${datarootdir}/doc/${PACKAGE_TARNAME}' which are expanded by make or pkg-config at runtime (that's why we have that stuff with AS_AC_EXPAND).
Comment on attachment 118617 [details] [review] Assume that DBUS_DATADIR is absolute on Windows Review of attachment 118617 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 118618 [details] [review] cmake: simplify definition of installation paths Review of attachment 118618 [details] [review]: ----------------------------------------------------------------- looks good
Created attachment 118652 [details] [review] Make sure that dbus library always uses the ansi version of GetModuleFileName()
(In reply to Simon McVittie from comment #47) > GetModuleFileName() means either GetModuleFileNameA() (some ANSI code page) > or GetModuleFileNameW() ("wide", i.e. UCS-2), and as far as I'm aware, we > consistently use the ANSI versions, which return a string encoded according > to the system code-page. in libdbus there is one reference to GetModuleFileName(). I made it sure that is uses the ansi version in any case, see attachment 118652 [details] [review] in dbus-launch-win.c there is GetModuleFileNameW (NULL, dbusDaemonPath, DIM (dbusDaemonPath)); ... if ((p = wcsrchr (dbusDaemonPath, L'\\')))
(In reply to Simon McVittie from comment #44) > (In reply to Ralf Habacker from comment #43) > > While reviewing I got the impression that having a dbus internal C++ variant > > of DBusString would reduce the required numbers of dbus_string_free() calls > > It's an interesting idea, but no, I'm not going to make libdbus C++. I did not say to provide a libdbus api c++. libdbus on windows already requires a c++ compiler, so the windows part could benefit from c++ advantages without additional costs and you are not been forced to depend on libstdc++ see, http://ptspts.blogspot.de/2010/12/how-to-write-c-program-without-libstdc.html.
Comment on attachment 118652 [details] [review] Make sure that dbus library always uses the ansi version of GetModuleFileName() Review of attachment 118652 [details] [review]: ----------------------------------------------------------------- > Make sure that dbus library always uses the ansi version of GetModuleFileName() "Better to be explicit than implicit", so the code change is fine, but I don't think your justification for it in the commit message is correct. As far as I know, GetModuleFileName() always expands to either GetModuleFileNameA(), which returns an ANSI string (8-bit, ASCII-compatible but possibly multi-byte) as a char *, or GetModuleFileNameW(), which returns a wide string (UTF-16 Unicode) as a wchar_t *. There is no third Windows API that somehow returns some other 8-bit string that isn't ANSI. The string manipulation functions are: * strfoo() (which works in terms of 8-bit bytes) * _mbstrfoo() (which works in terms of possibly multibyte characters, each consisting of one or more 8-bit units) * wstrfoo() (which works in terms of Unicode) Or am I confused?
(In reply to Ralf Habacker from comment #54) > I did not say to provide a libdbus api c++. libdbus on windows already > requires a c++ compiler, so the windows part could benefit from c++ > advantages without additional costs and you are not been forced to depend on > libstdc++ I know, but I still don't want to go this route for things that are possible in C. The majority of libdbus would still have to use the C DBusString anyway, so it seems more confusing and error-prone to have a secondary C++ version that is sometimes used. I was willing to make an exception for the thread initialization stuff because it didn't seem to be possible to get the same thread-safety any other way, and because it's isolated to one small module which presents a C API to the rest of D-Bus. I think Bug #89104 is a better route for making DBusString easier to work with.
(In reply to Simon McVittie from comment #55) > As far as I know, GetModuleFileName() always expands to either > GetModuleFileNameA(), which returns an ANSI string [...], > or GetModuleFileNameW(), which returns a wide string I think it's basically /* "ANSI" */ char *GetSomethingA (...); /* "Wide" (Unicode) */ wchar_t *GetSomethingW (...); #ifdef UNICODE #define GetSomething GetSomethingW #else #define GetSomething GetSomethingA #endif so the fact that we're doing char *foo = GetSomething (...); without compiler errors implies that we must already be in the ANSI case.
(In reply to Ralf Habacker from comment #43) > Looks good except the two mentioned issues. > Adding DBUS_DATADIR, which is expanded to an absolute configure time path, > to the runtime install root will result into an invalid path. Fixed in Attachment #118617 [details] and Attachment #118618 [details], I think. > Not your change, but the path is returned from the non multi byte version of > GetModuleFileName. It looks wrong to use this multi byte function to search > for the last slash. I think this is already correct, because GetModuleFileName() is returning a char *, which means it must be GetModuleFileNameA() behind the scenes, so _mbsrchr() is right. If it was GetModuleFileNameW() then the compiler would complain that we were trying to assign a wchar_t * to a char *. I wouldn't object to swapping it to strrchr() if there's a reason to prefer strrchr() and you're confident that it won't break anything. D-Bus has a pervasive assumption that 8-bit strings are UTF-8 (the same assumption is present in GLib), so ideally we should be using GetFooW() and using Windows APIs to convert the resulting wchar_t * from UTF-16 to UTF-8 - but I suspect that would be a lot of code for very little benefit. Given that this isn't a regression, can I merge what I have so far while we discuss the rest?
(In reply to Simon McVittie from comment #58) > Given that this isn't a regression, can I merge what I have so far while we > discuss the rest? yes, see comment 50 and comment 51
Comment on attachment 118540 [details] [review] [untested] Use DBusString for all relocation and install-root code Tested and reviewed by Ralf, and pushed to master for 1.11.0
Comment on attachment 118617 [details] [review] Assume that DBUS_DATADIR is absolute on Windows Committed to master for 1.11.0
Comment on attachment 118618 [details] [review] cmake: simplify definition of installation paths Committed to master for 1.11.0
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/110.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.