Running test-autolaunch in a native windows or cross compiled build dir fails with the following error message: bin/test-autolaunch.exe Failed to start message bus: Failed to open "Z:/xxx/src/dbus-1-cmake-cross-x86-build/share/dbus-1/session.conf": path not found. The test suite is intended to be runned from the build dir which requires to generate session.conf in <build-root>/share/dbus-1
Created attachment 119691 [details] [review] Fix test-autolaunch test by generating session.conf in <build-dir>/share/dbus-1 for cmake.
Comment on attachment 119691 [details] [review] Fix test-autolaunch test by generating session.conf in <build-dir>/share/dbus-1 for cmake. Review of attachment 119691 [details] [review]: ----------------------------------------------------------------- ::: cmake/bus/CMakeLists.txt @@ +4,4 @@ > SET(BUS_DIR ${CMAKE_SOURCE_DIR}/../bus) > > # config files for installation > +CONFIGURE_FILE( "${BUS_DIR}/session.conf.in" "${CMAKE_BINARY_DIR}/share/dbus-1/session.conf" IMMEDIATE @ONLY) This is going to break "make install", which relies on the previous naming: install(FILES ${CMAKE_CURRENT_BINARY_DIR}/session.conf DESTINATION share/dbus-1) ... if(NOT WIN32) install(FILES ${CMAKE_CURRENT_BINARY_DIR}/system.conf DESTINATION share/dbus-1) endif() You could either create both, or also change what gets installed.
(In reply to Simon McVittie from comment #2) > Comment on attachment 119691 [details] [review] [review] > Fix test-autolaunch test by generating session.conf in > <build-dir>/share/dbus-1 for cmake. > > Review of attachment 119691 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: cmake/bus/CMakeLists.txt > @@ +4,4 @@ > > SET(BUS_DIR ${CMAKE_SOURCE_DIR}/../bus) > > > > # config files for installation > > +CONFIGURE_FILE( "${BUS_DIR}/session.conf.in" "${CMAKE_BINARY_DIR}/share/dbus-1/session.conf" IMMEDIATE @ONLY) > > This is going to break "make install", which relies on the previous naming: > got it. I guess this issue affects also tests build with autotools. Any idea how to generated session.conf in the new location there ?
Created attachment 119735 [details] [review] Fix test-autolaunch test by generating session.conf in <build-dir>/share/dbus-1 for cmake.
(In reply to Ralf Habacker from comment #3) > I guess this issue affects also tests build with autotools. Any idea how to > generated session.conf in the new location there ? Because test-autolaunch is part of the "embedded tests", I think we could dodge the whole issue by making it look for the DBUS_TEST_DATA environment variable. On Unix, it's dbus-launch (not dbus-sysdeps-unix.c) that specifies the dbus-daemon's command line. However, none of the tests in test/name-test are designed to be run directly in a built tree anyway: they rely on special "magic" environment variables. In Unix, they're run by test/name-test/run-test.sh, which indirectly uses dbus-run-session. The multiprocessing model is sufficiently different on Windows (no fork()) that dbus-run-session doesn't necessarily make any sense there. I don't think test/name-test/ is applicable to non-Autotools, non-Unix builds without significant work. I'd prefer to add enough test coverage to the GLib-style tests that test/name-test/ is no longer relevant, and then delete test/name-test/ altogether.
Created attachment 119741 [details] [review] Windows _dbus_get_autolaunch_address: don't leak shm_name
Created attachment 119742 [details] [review] Windows _dbus_get_autolaunch_address: use DBUS_TEST_DATA etc. The Unix equivalent is in tools/dbus-launch.c, because X11 autolaunching works differently.
(In reply to Simon McVittie from comment #5) > Because test-autolaunch is part of the "embedded tests", I think we could > dodge the whole issue by making it look for the DBUS_TEST_DATA environment > variable. With the patches I attached, DBUS_TEST_DAEMON=Z:/srv/tmp/smcv/build/dbus/mingw/bus/dbus-daemon.exe DBUS_TEST_DATA=Z:/srv/tmp/smcv/build/dbus/mingw/test/data DBUS_USE_TEST_BINARY=1 /srv/tmp/smcv/build/dbus/mingw/test/name-test/test-autolaunch.exe seems to work. But it leaks two dbus-daemon processes, and I don't think that can be fixed, because there is nothing to scope the lifetime of the dbus-daemon like there is under X11. I don't think we should run any of the tests in test/name-test/ under CMake or when cross-compiling.
Created attachment 119743 [details] [review] name-test: don't run these tests if targeting Windows The wrapper shell script that sets up their environment is nowhere near being portable. In particular, it uses dbus-run-session, which is Unix-specific.
Comment on attachment 119741 [details] [review] Windows _dbus_get_autolaunch_address: don't leak shm_name Review of attachment 119741 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 119742 [details] [review] Windows _dbus_get_autolaunch_address: use DBUS_TEST_DATA etc. Review of attachment 119742 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 119742 [details] [review] Windows _dbus_get_autolaunch_address: use DBUS_TEST_DATA etc. Review of attachment 119742 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +3123,5 @@ > goto out; > } > > +#ifdef DBUS_ENABLE_EMBEDDED_TESTS > + if (_dbus_getenv ("DBUS_USE_TEST_BINARY") != NULL) After thinking twice: Is is really required to introduce an additional environment variable ? Would not checking presence of DBUS_TEST_DAEMON enough as already done in test-utils-glib.c ? Looks that there are different approaches: two times directly using DBUS_TEST_DAEMON and two times not.
Created attachment 119921 [details] [review] Add windows implementation of dbus-run-session helper tool.
Comment on attachment 119741 [details] [review] Windows _dbus_get_autolaunch_address: don't leak shm_name committed to dbus-1.10
Comment on attachment 119921 [details] [review] Add windows implementation of dbus-run-session helper tool. Review of attachment 119921 [details] [review]: ----------------------------------------------------------------- Design: The purpose of dbus-user-session (see its man page) is that it never reuses an existing dbus-daemon, and always starts a new one - this means that autolaunch: is not a suitable transport, because autolaunch: always tries to reuse an existing dbus-daemon. On Unix, making the assumption that session.conf contains unix:tmpdir=/tmp is probably fine (although it's arguably a minor bug that the Unix code path doesn't force --address=unix:tmpdir=/tmp in case session.conf is configured for something else), but on Windows, we can assume that what's in session.conf is *not* suitable. Instead, this should pass --address= to dbus-daemon on Windows to force a specific listenable address (--address=nonce-tcp:host=127.0.0.1 would probably be best), use a pipe as the subprocess's stdout, and use --print-address=1 to write the connectable address into the pipe - a lot like spawn_dbus_daemon() in test-utils-glib.c, but without using GLib. Then dbus-run-session should read the connectable address out of the pipe and use it as the app's DBUS_SESSION_BUS_ADDRESS, like it does on Unix. In the Unix code path, the listenable address in session.conf is always unix:tmpdir=/tmp in practice, and the connectable address is something like unix:abstract=/tmp/dgliuglefg. In the Windows code path, the listenable address should probably be nonce-tcp:host=127.0.0.1 and the connectable address would be something like nonce-tcp:host=127.0.0.1,port=55892. ---- Implementation: I'm surprised this compiles, because it uses read() and ssize_t. I thought Windows code had to use _read() and int? You'll probably need something very similar to the read-until-newline loop from spawn_dbus_daemon(), but reading into a DBusString or a fixed-size buffer instead of into GLib's GString. ::: cmake/tools/CMakeLists.txt @@ +77,5 @@ > target_link_libraries(dbus-monitor ${DBUS_LIBRARIES}) > install_targets(/bin dbus-monitor ) > > +add_executable(dbus-run-session ../../tools/dbus-run-session.c) > +target_link_libraries(dbus-run-session ${DBUS_LIBRARIES} dbus-internal) If you're going to enable this cross-platform, please make it unconditional in tools/Makefile.am as well. At the moment it's conditional on DBUS_UNIX. ::: dbus/dbus-spawn.h @@ +63,5 @@ > void *data, > DBusFreeFunction free_data_function); > > +HANDLE > +_dbus_spawn_program (char* name, char** argv, char** envp); This will not compile on Unix. HANDLE is Windows-specific. You could declare this in dbus/dbus-sysdeps-win.h with a comment /* in dbus-spawn.c */, or you could inline the implementation of spawn_program() into dbus-run-session.c (I suspect that you might need to copy and modify spawn_program() to make it use --print-address anyway). ::: tools/dbus-run-session.c @@ +205,5 @@ > exit (1); > } > > +static int > +run_session(char *dbus_daemon, char *config_file, char *bus_address, char **argv, int prog_arg) dbus_daemon and config_file should be const. I'm surprised the compiler didn't reject this. @@ +229,5 @@ > + > + if (bus_pid == 0) > + { > + /* child */ > + exec_dbus_daemon (dbus_daemon, bus_address_pipe, config_file); If exec_dbus_daemon() and exec_app() aren't used on Windows, they should be #ifdef DBUS_UNIX. (Does Windows even have execlp(), execvp()?) @@ +341,5 @@ > + char *env[3]; > + int ret = 127; > + char *s; > + HANDLE hProcess; > + HANDLE hChild; I'd prefer these to be called bus_process and app_process or something, consistent with the bus_pid and app_pid naming in the Unix code path. It isn't immediately obvious that hProcess is the dbus-daemon, and hChild is the app or regression test. @@ +342,5 @@ > + int ret = 127; > + char *s; > + HANDLE hProcess; > + HANDLE hChild; > + DWORD exitCode; D-Bus coding style is words_with_underscores, not camelCase. @@ +345,5 @@ > + HANDLE hChild; > + DWORD exitCode; > + > + snprintf (buffer[0], sizeof(buffer[0]), "%s", dbus_daemon); > + snprintf (buffer[1], sizeof(buffer[1]), "--config-file=%s", config_file); Please use DBusString instead of snprintf() into a buffer. (I know one of the Unix functions in this file already does a snprintf() of an integer into a buffer - at the time I wrote it, we couldn't use _dbus functions in our tools without statically linking libdbus-internal, but now we can. Also, the length of a fixed-bit-size integer is bounded.) @@ +356,5 @@ > + > + hProcess = _dbus_spawn_program (dbus_daemon, argv2, env); > + if (!hProcess) > + { > + fprintf (stderr, "%s: could not be started error=%ld\n", dbus_daemon, GetLastError ()); Can't we have some sort of equivalent of strerror() instead? I would expect something more like fprintf (stderr, "%s: failed to start %s: %s", me, dbus-daemon, some_equivalent_of_strerror (GetLastError ())); so that the output would look something like dbus-run-session: failed to start dbus-daemon.exe: No such file or directory (From 30 seconds on MSDN, FormatMessage() might be the right function to use, but I could be wrong.) @@ +360,5 @@ > + fprintf (stderr, "%s: could not be started error=%ld\n", dbus_daemon, GetLastError ()); > + goto out; > + } > + > + snprintf (buffer[0], sizeof(buffer[0]), "DBUS_SESSION_BUS_ADDRESS=autolaunch:scope=*install-path"); This should be the connectable address that was printed into the pipe by --print-address=1 (read it into a buffer or a DBusString). @@ +367,5 @@ > + > + s = getenv("DBUS_VERBOSE"); > + if (s) > + { > + snprintf (buffer[1], sizeof (buffer[1]), "DBUS_VERBOSE=%s", s && *s == '1' ? "1" : "0"); This is setting DBUS_SESSION_BUS_ADDRESS, copying DBUS_VERBOSE, and omitting the rest of our environment, which seems wrong. I would expect it to iterate over the environment, copying every variable except for DBUS_SESSION_BUS_ADDRESS and the ones the Unix code path unsets (DBUS_SESSION_BUS_PID and so on), and then append DBUS_SESSION_BUS_ADDRESS. For instance, if PATH is set in dbus-run-session's environment, it should be set to the same value in the app. @@ +375,5 @@ > + > + hChild = _dbus_spawn_program (argv[prog_arg], argv + prog_arg, env); > + if (!hChild) > + { > + fprintf (stderr, "%s: child process died or something error=%ld\n", argv[prog_arg], GetLastError ()); Can't we have some sort of equivalent of strerror() instead? I would expect something like fprintf (stderr, "%s: failed to start %s: %s", me, argv[prog_arg], some_equivalent_of_strerror (GetLastError ())); so that the output would look something like dbus-run-session: failed to start pidgin.exe: No such file or directory (From 30 seconds on MSDN, FormatMessage() might be the right function to use, but I could be wrong.) @@ +385,5 @@ > + goto out; > + ret = exitCode; > + > +out: > + TerminateProcess (hProcess, 0); Is this safe to do if the dbus-daemon has already exited? On Unix, the equivalent might kill the wrong process if the process ID has been reused (but maybe Windows process handles are immune to this). @@ +400,3 @@ > const char *config_file = NULL; > const char *dbus_daemon = NULL; > char bus_address[MAX_ADDR_LEN] = { 0 }; bus_address is only populated in the Unix code path, so it should be declared in run_session() instead of being passed as an argument.
(In reply to Ralf Habacker from comment #12) > After thinking twice: Is is really required to introduce an additional > environment variable ? Would not checking presence of DBUS_TEST_DAEMON > enough as already done in test-utils-glib.c ? > > Looks that there are different approaches: two times directly using > DBUS_TEST_DAEMON and two times not. DBUS_USE_TEST_BINARY is the old approach; it had the disadvantage that the actual path to the test binary had to be hard-coded into the executables. I think it would be OK to assume that if DBUS_TEST_DAEMON is set, we should use it, and if it is not, we should do the normal thing (a PATH search). This might require finding and fixing some places that set DBUS_USE_TEST_BINARY but do not set DBUS_TEST_DAEMON, if there are any.
Comment on attachment 119743 [details] [review] name-test: don't run these tests if targeting Windows committed to dbus-1.10
Related URL: http://localhost:8080/11 (Gerrit Patch-Set I5125f8b1baefeb71d40b50953982bc1a405ef932/1)
Related URL: http://localhost:8080/gitweb?p=dbus.git;a=commit;h=61eb213af240e13fc3f72d75dff6d2392fda187a (Git: 61eb213af240e13fc3f72d75dff6d2392fda187a)
(In reply to Ralf Habacker from comment #18) > Related URL: http://localhost:8080/11 (Gerrit Patch-Set > I5125f8b1baefeb71d40b50953982bc1a405ef932/1) Sorry for the noise. I tried to evaluate gerrit [1] and its bugzilla freedesktop integration on a local machine to see if we can ease dbus (and maybe additional freedesktop projects [2]) review process and configured to only log actions related to freedeektop bugzilla. For that I used commits from this bug to test the review process. Unfortunally gerrits bugzilla plugin seems to have a hidden default action to push comments to related bugs. [1] gerrit https://en.wikipedia.org/wiki/Gerrit_%28software%29 [2] freedesktop libreoffice project already uses gerrit as review system https://gerrit.libreoffice.org
Gerrit is an interesting option, but I'd prefer continuing to use the Splinter integration in Bugzilla for dbus, at least in the short to medium term. Our requirements for code review infrastructure are not particularly elaborate, and Bugzilla is a well-understood "lowest common denominator". It's also the same thing used for patch review in GNOME, and for bug tracking in GNOME, KDE and assorted other free desktop environments. freedesktop.org also has an installation of Phabricator (integrated bug tracking, patch review, project management etc.), which is what my employer Collabora uses for various projects. I certainly don't want to rush into that, though.
(In reply to Simon McVittie from comment #15) > > Instead, this should pass --address= to dbus-daemon on Windows to force a > specific listenable address (--address=nonce-tcp:host=127.0.0.1 would > probably be best), use a pipe as the subprocess's stdout, and use > --print-address=1 to write the connectable address into the pipe - a lot > like spawn_dbus_daemon() in test-utils-glib.c, but without using GLib. Then > dbus-run-session should read the connectable address out of the pipe and use > it as the app's DBUS_SESSION_BUS_ADDRESS, like it does on Unix. Unfortunally this does not work out of the box and requires specific code on the child process (search for 'child process' at https://msdn.microsoft.com/de-de/library/windows/desktop/ms682499%28v=vs.85%29.aspx). dbus on windows currently supports a kind of pipe to get the session bus address by the autolaunch protocol scope paramter. > - this means that autolaunch: is not a suitable transport, because autolaunch: > always tries to reuse an existing dbus-daemon. If a daemon is started by dbus-run-session with a dedicated autolaunch scope (which may be '*install-path' or '1234556' or 'test-suite' libdbus will not start an additional daemon, but will use the one started by dbus-run-session.
(In reply to Ralf Habacker from comment #22) > If a daemon is started by dbus-run-session with a dedicated autolaunch scope > (which may be '*install-path' or '1234556' or 'test-suite' libdbus will not > start an additional daemon, but will use the one started by dbus-run-session. Running the same dbus-run-session.exe twice in parallel must result in two separate "sessions", each with their own unique DBUS_SESSION_BUS_ADDRESS which is passed to their child processes. Perhaps you could use _dbus_create_uuid() to end up with something like "autolaunch:scope=11223344556677889900aabbccddeeff", and put that in both the dbus-daemon's --address argument, and the app's DBUS_SESSION_BUS_ADDRESS? I don't want to build and install dbus-run-session unless it has the same behaviour as it does on Unix (see the man page), because the reason I introduced it in the first place was that dbus-launch has various intertwined purposes that make it hard to reason about. > Unfortunally this does not work out of the box and requires specific code on > the child process (search for 'child process' at > https://msdn.microsoft.com/de-de/library/windows/desktop/ms682499%28v=vs. > 85%29.aspx). Hmm. It looks as though GLib does this by running a separate helper executable to shuffle file handles around. If the autolaunch thing doesn't work out, one other possibility would be to use the same g_spawn-based setup as test-utils-glib.c, and compile dbus-run-session if either we are compiling for Unix, or we have GLib: if DBUS_UNIX bin_PROGRAMS += dbus-run-session else if DBUS_WITH_GLIB # on Windows it can exist, but requires GLib bin_PROGRAMS += dbus-run-session endif endif
(In reply to Simon McVittie from comment #21) > freedesktop.org also has an installation of Phabricator (integrated bug > tracking, patch review, project management etc.), which is what my employer > Collabora uses for various projects. Did a search and got some infos: 1. A feature compare between phabricator and gerrit https://community.kde.org/Sysadmin/FutureInfrastructure 2. KDE is also migrating to phabricator https://techbase.kde.org/Development/Phabricator
(In reply to Simon McVittie from comment #23) > (In reply to Ralf Habacker from comment #22) > > If a daemon is started by dbus-run-session with a dedicated autolaunch scope > > (which may be '*install-path' or '1234556' or 'test-suite' libdbus will not > > start an additional daemon, but will use the one started by dbus-run-session. > > Running the same dbus-run-session.exe twice in parallel must result in two > separate "sessions", each with their own unique DBUS_SESSION_BUS_ADDRESS > which is passed to their child processes. Perhaps you could use > _dbus_create_uuid() to end up with something like > "autolaunch:scope=11223344556677889900aabbccddeeff", That should work. Unfortunally is _dbus_create_uuid not available on windows: from file dbus/dbus-uuidgen.c #ifdef DBUS_WIN #error "dbus-uuidgen should not be needed on Windows" #endif ... dbus_bool_t _dbus_create_uuid (char **uuid_p, DBusError *error)
Created attachment 120166 [details] [review] Rename function string_array_length() to _dbus_string_array_length() and move it to dbus-internals.c. This function is required by dbus-run-session on windows. Bug:
Created attachment 120169 [details] [review] Add windows implementation of dbus-run-session helper tool. The implementation requires a dbus-daemon configured with 'autolaunch:scope=*install-path' listen address, which let the client find the session bus address from shared memory. Bug:
Created attachment 120170 [details] [review] Add new functions _dbus_string_array_replace() _dbus_string_array_append(). These functions are required by dbus-run-session on windows. Bug:
(In reply to Simon McVittie from comment #15) > Implementation: > > I'm surprised this compiles, because it uses read() and ssize_t. I thought > Windows code had to use _read() and int? fixed > ::: cmake/tools/CMakeLists.txt > @@ +77,5 @@ > > target_link_libraries(dbus-monitor ${DBUS_LIBRARIES}) > > install_targets(/bin dbus-monitor ) > > > > +add_executable(dbus-run-session ../../tools/dbus-run-session.c) > > +target_link_libraries(dbus-run-session ${DBUS_LIBRARIES} dbus-internal) > > If you're going to enable this cross-platform, please make it unconditional > in tools/Makefile.am as well. At the moment it's conditional on DBUS_UNIX. next patch > ::: dbus/dbus-spawn.h > @@ +63,5 @@ > > void *data, > > DBusFreeFunction free_data_function); > > > > +HANDLE > > +_dbus_spawn_program (char* name, char** argv, char** envp); > > This will not compile on Unix. HANDLE is Windows-specific. > > You could declare this in dbus/dbus-sysdeps-win.h with a comment /* in > dbus-spawn.c */, or you could inline the implementation of spawn_program() > into dbus-run-session.c (I suspect that you might need to copy and modify > spawn_program() to make it use --print-address anyway). need to fix > ::: tools/dbus-run-session.c > @@ +205,5 @@ > > exit (1); > > } > > > > +static int > > +run_session(char *dbus_daemon, char *config_file, char *bus_address, char **argv, int prog_arg) > > dbus_daemon and config_file should be const. I'm surprised the compiler > didn't reject this. fixed > > @@ +229,5 @@ > > + > > + if (bus_pid == 0) > > + { > > + /* child */ > > + exec_dbus_daemon (dbus_daemon, bus_address_pipe, config_file); > > If exec_dbus_daemon() and exec_app() aren't used on Windows, they should be > #ifdef DBUS_UNIX. fixed > > (Does Windows even have execlp(), execvp()?) it has > @@ +341,5 @@ > > + char *env[3]; > > + int ret = 127; > > + char *s; > > + HANDLE hProcess; > > + HANDLE hChild; > > I'd prefer these to be called bus_process and app_process or something, > consistent with the bus_pid and app_pid naming in the Unix code path. It > isn't immediately obvious that hProcess is the dbus-daemon, and hChild is > the app or regression test. fixed > > @@ +342,5 @@ > > + int ret = 127; > > + char *s; > > + HANDLE hProcess; > > + HANDLE hChild; > > + DWORD exitCode; > > D-Bus coding style is words_with_underscores, not camelCase. fixed > > @@ +345,5 @@ > > + HANDLE hChild; > > + DWORD exitCode; > > + > > + snprintf (buffer[0], sizeof(buffer[0]), "%s", dbus_daemon); > > + snprintf (buffer[1], sizeof(buffer[1]), "--config-file=%s", config_file); > > Please use DBusString instead of snprintf() into a buffer. fixed > @@ +356,5 @@ > > + > > + hProcess = _dbus_spawn_program (dbus_daemon, argv2, env); > > + if (!hProcess) > > + { > > + fprintf (stderr, "%s: could not be started error=%ld\n", dbus_daemon, GetLastError ()); > > Can't we have some sort of equivalent of strerror() instead? I would expect > something more like fixed > @@ +360,5 @@ > > + fprintf (stderr, "%s: could not be started error=%ld\n", dbus_daemon, GetLastError ()); > > + goto out; > > + } > > + > > + snprintf (buffer[0], sizeof(buffer[0]), "DBUS_SESSION_BUS_ADDRESS=autolaunch:scope=*install-path"); > > This should be the connectable address that was printed into the pipe by > --print-address=1 (read it into a buffer or a DBusString). fixed > > @@ +367,5 @@ > > + > > + s = getenv("DBUS_VERBOSE"); > > + if (s) > > + { > > + snprintf (buffer[1], sizeof (buffer[1]), "DBUS_VERBOSE=%s", s && *s == '1' ? "1" : "0"); > > This is setting DBUS_SESSION_BUS_ADDRESS, copying DBUS_VERBOSE, and omitting > the rest of our environment, which seems wrong. > > I would expect it to iterate over the environment, copying every variable > except for DBUS_SESSION_BUS_ADDRESS and the ones the Unix code path unsets > (DBUS_SESSION_BUS_PID and so on), and then append DBUS_SESSION_BUS_ADDRESS. > For instance, if PATH is set in dbus-run-session's environment, it should be > set to the same value in the app. fixed > > @@ +375,5 @@ > > + > > + hChild = _dbus_spawn_program (argv[prog_arg], argv + prog_arg, env); > > + if (!hChild) > > + { > > + fprintf (stderr, "%s: child process died or something error=%ld\n", argv[prog_arg], GetLastError ()); > > Can't we have some sort of equivalent of strerror() instead? I would expect > something like fixed > @@ +385,5 @@ > > + goto out; > > + ret = exitCode; > > + > > +out: > > + TerminateProcess (hProcess, 0); > > Is this safe to do if the dbus-daemon has already exited? yes > On Unix, the > equivalent might kill the wrong process if the process ID has been reused > (but maybe Windows process handles are immune to this). it is no pid. > @@ +400,3 @@ > > const char *config_file = NULL; > > const char *dbus_daemon = NULL; > > char bus_address[MAX_ADDR_LEN] = { 0 }; > > bus_address is only populated in the Unix code path, so it should be > declared in run_session() instead of being passed as an argument. fixed
Created attachment 120172 [details] [review] Fixup - move dbus_spawn_program to dbus-sysdeps-win.h Will be merged into main commit. Bug:
I'll try to re-review this soon, but I can't promise anything. Tagging it as having patches so it is at least more visible.
Comment on attachment 120169 [details] [review] Add windows implementation of dbus-run-session helper tool. Review of attachment 120169 [details] [review]: ----------------------------------------------------------------- ::: cmake/tools/CMakeLists.txt @@ +77,5 @@ > target_link_libraries(dbus-monitor ${DBUS_LIBRARIES}) > install_targets(/bin dbus-monitor ) > > +add_executable(dbus-run-session ../../tools/dbus-run-session.c) > +target_link_libraries(dbus-run-session ${DBUS_LIBRARIES} dbus-internal) It's still only compiled for Unix under Autotools. If it's reasonably easy, can we link it to libdbus-internal when building for Windows or libdbus otherwise? Or we could just link it to libdbus-internal either way (but if so, please do that under Autotools as well). ::: dbus/dbus-spawn-win.c @@ +566,5 @@ > +#ifdef DBUS_ENABLE_VERBOSE_MODE > + { > + char *s = compose_string (envp, ';'); > + _dbus_verbose ("spawning '%s'' with args: '%s' env: '%s'\n", name, arg_string, s); > + free (s); Indentation seems to have gone a bit weird here (one more space before _dbus_verbose, unless Splinter is misleading me) @@ +591,5 @@ > +static HANDLE > +spawn_program (char* name, char** argv, char** envp) > +{ > + return _dbus_spawn_program (name, argv, envp); > +} This only seems to have one caller... instead of adding this shim, why not just change that caller? ::: dbus/dbus-spawn.h @@ +63,5 @@ > void *data, > DBusFreeFunction free_data_function); > > +HANDLE > +_dbus_spawn_program (const char* name, char** argv, char** envp); No newline between return-type and name for declarations (with ";"), only for definitions (with "{}"). The idea is that if we do that consistently, we can search for something like "^_dbus_spawn_program\>" to go directly to the definition. ::: tools/dbus-run-session.c @@ +41,2 @@ > #include "dbus/dbus.h" > +#include <dbus/dbus-spawn.h> Surely it should now (with Attachment #120172 [details]) look more like this? #ifdef DBUS_UNIX #include <sys/wait.h> #include <signal.h> #else #include "dbus/dbus-sysdeps-win.h" #endif (I'm assuming you're going to squash Attachment #120172 [details] into this next time you revise it.) @@ +337,5 @@ > + } > + > + return 0; > +#else > + char *argv2[5]; This name is rather nonspecific. dbus_daemon_argv? Why length 5? You only seem to use 4 entries, 0..3. @@ +342,5 @@ > + int ret = 127; > + HANDLE server_handle; > + HANDLE app_handle; > + DWORD exit_code; > + DBusString string[4]; argv_strings? @@ +351,5 @@ > + _dbus_string_init (&string[2]); > + _dbus_string_init (&string[3]); > + > + /* run dbus daemon */ > + _dbus_string_append_printf (&string[3], "autolaunch:scope=dbus-tmp-session-%d", getpid()); Does a genuine Windows development environment have getpid(), or is that a mingw thing? I would have expected that you'd have to use GetCurrentProcessId()? Is %d (signed int) the correct type for a Windows process ID? I would still prefer this to be something a bit more unique, like the result of UuidToString() on UuidCreate(), but I suppose a pid is good enough unless someone is actively trying to cause DoS. @@ +358,5 @@ > + _dbus_string_append_printf (&string[2], "--address=%s", _dbus_string_get_const_data (&string[3])); > + argv2[0] = _dbus_string_get_data (&string[0]); > + argv2[1] = _dbus_string_get_data (&string[1]); > + argv2[2] = _dbus_string_get_data (&string[2]); > + argv2[3] = 0; Please use NULL for null pointers, not 0. I know 0 is preferred in some C++ style guides, but this is C. If you made it length 4, you could use dbus_daemon_argv[0] = ...; ... dbus_daemon_argv[3] = NULL; _DBUS_STATIC_ASSERT (_DBUS_N_ELEMENTS (dbus_daemon_argv) == 4); to make it obvious that the length has been set correctly. @@ +360,5 @@ > + argv2[1] = _dbus_string_get_data (&string[1]); > + argv2[2] = _dbus_string_get_data (&string[2]); > + argv2[3] = 0; > + > + server_handle = _dbus_spawn_program (dbus_daemon, argv2, 0); Please use NULL for null pointers, not 0. @@ +363,5 @@ > + > + server_handle = _dbus_spawn_program (dbus_daemon, argv2, 0); > + if (!server_handle) > + { > + _dbus_win_warn_win_error ("Could not start dbus daemon", GetLastError ()); I'd somewhat prefer this to not use _dbus_warn(), because _dbus_warn() causes an abort() by default ("worse than" an unsuccessful exit). Can we use something more like the format in which the Unix case does its output, which is something like dbus-run-session: unable to do whatever: No such file or directory to stderr? In this case I'd expect something like "%s: could not start dbus-daemon: %s" where the arguments are the global variable 'me' (basically argv[0]), and something involving applying FormatMessage() to GetLastError(). @@ +372,5 @@ > + _dbus_string_append_printf (&string[0], "DBUS_SESSION_BUS_ADDRESS=%s", _dbus_string_get_data (&string[3])); > + > + env = _dbus_get_environment (); > + if (!_dbus_string_array_replace ((const char **) env, "DBUS_SESSION_BUS_ADDRESS", _dbus_string_get_data (&string[0]))) > + if (!_dbus_string_array_append (env, _dbus_string_get_data (&string[0]))) This API seems odd for a generic "string array" API. I'll say more about that when reviewing these added functions. @@ +373,5 @@ > + > + env = _dbus_get_environment (); > + if (!_dbus_string_array_replace ((const char **) env, "DBUS_SESSION_BUS_ADDRESS", _dbus_string_get_data (&string[0]))) > + if (!_dbus_string_array_append (env, _dbus_string_get_data (&string[0]))) > + oom(); I'd prefer {} around the body of the outer "if". @@ +378,5 @@ > + > + app_handle = _dbus_spawn_program (argv[prog_arg], argv + prog_arg, env); > + if (!app_handle) > + { > + _dbus_win_warn_win_error ("child process died or something", GetLastError ()); The message "child process died or something" in the Unix case is for when we can't parse the child's exit status into anything sensible. Something like "unable to start child process" might make sense here?
Comment on attachment 120166 [details] [review] Rename function string_array_length() to _dbus_string_array_length() and move it to dbus-internals.c. Review of attachment 120166 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-internals.c @@ +650,5 @@ > +size_t > +_dbus_string_array_length (const char **array) > +{ > + size_t i; > + for (i = 0; array[i]; i++) ; While you're moving this around anyway, I'd prefer the empty body to be {} rather than ; (to make it a little more visible).
Comment on attachment 120170 [details] [review] Add new functions _dbus_string_array_replace() _dbus_string_array_append(). Review of attachment 120170 [details] [review]: ----------------------------------------------------------------- I'd be inclined to include (the replacement for) these in the same commit as the Windows version of dbus-run-session - they're no use until that's added, and when adding new APIs it's often helpful to see an example of their real-world use. ::: dbus/dbus-internals.c @@ +660,5 @@ > + * > + * @param array array to search. > + * @param search string to search for. > + * @returns replace string to replace. > + * @returns TRUE if search string has been found, FALSE otherwise. If this returns FALSE, you can't tell whether that means "search string not found" or "search string found, but then we ran out of memory". See below for more comments on error-handling. @@ +668,5 @@ > +{ > + size_t i, len = _dbus_string_array_length (array); > + for (i=0 ; i < len; i++) > + { > + if (strstr (array[i], search)) This searches for any string that *contains* @search, which is quite likely to do the wrong thing. Pseudocode: _dbus_string_array_replace ({ "FOO=BAR", "BAZ=BADGER", NULL }, "BAD", "BAD=LUCK") would unexpectedly replace $BAZ with BAD=LUCK. Similarly, _dbus_string_array_replace ({ "FOO=23", "BARRACUDA=a big fish", NULL }, "BAR", "BAR=42") would unexpectedly replace $BARRACUDA with BAR=42. I think this would be better if these two new functions were combined into one that looked more like g_environ_setenv(). (I might have got the Doxygen syntax wrong here.) /** * If @param env contains the same variable as @param var_equals_value, replace it with * a copy of @param var_equals_value. Otherwise append @param var_equals_value. * * @param env A NULL-terminated array of variable=value environment entries. If a non-#NULL value is returned, this replaces @param env, but if #NULL is returned, it does not (like realloc()). * @param var_equals_value A new variable in the form variable=value * @return The new value of @param env with @param var_equals_value included, or #NULL (without freeing @env) if no memory */ char ** _dbus_environ_setenv (char **env, const DBusString *var_equals_value) { /* find len (the same) */ int name_length; dbus_bool_t equals_found; const char *data = _dbus_string_get_const_data (var_equals_value); equals_found = _dbus_string_find (var_equals_value, 0, "=", &name_length)); _dbus_assert (equals_found); for (i = 0; i < len; i++) { if (strncmp (data, env[i], name_length + 1) == 0) { /* copy data */ /* return NULL if no memory */ dbus_free (env[i]); env[i] = copy; return env; } } /* realloc env 1 unit longer */ /* return NULL if no memory */ /* append a copy of data */ /* return env, or NULL if there was not enough memory */ } (Or var_equals_value could be a const char *, and you could use strchr() to find the equals.) @@ +673,5 @@ > + { > + char *s = _dbus_strdup (replace); > + if (!s) > + return FALSE; > + dbus_free ((char *)array[i]); See this cast? This is a sign that your function argument has the wrong type :-) @@ +684,5 @@ > + > +/** > + * Append string to a string array > + * > + * @param array array to append string. If an argument is "consumed" like this one, that's unusual enough that it should be specifically stated in the doc-comment: "If this function returns non-NULL, this argument is freed by this function and replaced by the returned array (which may be the same or different), like realloc()". @@ +686,5 @@ > + * Append string to a string array > + * > + * @param array array to append string. > + * @param append string to append. > + * @returns new string array or NULL in case of errors. There are three valid signatures for a libdbus function that can fail: /** On out-of-memory, return SOME_VALUE. */ ReturnType foo (...); /** On error, return SOME_VALUE and set error. */ ReturnType foo (..., DBusError *error); /** On error, return SOME_VALUE and set errno (or GetLastError() on Windows). */ ReturnType foo (...); If a function can only fail because it ran out of memory and not for any other reason, you can say "... or (NULL|false|-1|whatever) if no memory" and that's fine. This function seems to be in that category. If a function can fail for any reason other than OOM, you have to either use a DBusError (option 2), or for low-level functions only, say how the caller can report the failure (option 3). Otherwise, the caller can't distinguish between different errors. Existing libdbus functions that are in category 3 don't always specifically say how errors are reported (in practice usually errno or GetLastError()), but new functions should. @@ +698,5 @@ > + if (!array) > + return NULL; > + > + len = _dbus_string_array_length ((const char **) array); > + copy = dbus_new0 (char *, len +1); You can use dbus_realloc(), which will copy everything in @array and free @array, or not do anything if @array is actually already large enough behind the scenes; then you'd only need to overwrite array[len] with the new item, and array[len + 1] with the NULL.
(In reply to Simon McVittie from comment #8) > But it leaks two dbus-daemon processes, and I don't think > that can be fixed, because there is nothing to scope the lifetime of the > dbus-daemon like there is under X11. Because of this, I don't think test-autolaunch makes sense to run on Windows, even if you want to run the rest of name-test there. I'm not entirely convinced that test makes sense on Unix either - it hardly tests anything! One day I'll have time to write an integration test for an installed X11 dbus-launch, using autopkgtest or something, which can run in whole-system-installed environments like Debian's autopkgtest. The "meat" of Windows autolaunch is the autolaunch: transport, which would be adequately tested by running anything that's wrapped in your implementation of dbus-run-session - so let's do that instead.
(In reply to Simon McVittie from comment #16) > DBUS_USE_TEST_BINARY is the old approach; it had the disadvantage that the > actual path to the test binary had to be hard-coded into the executables. I > think it would be OK to assume that if DBUS_TEST_DAEMON is set, we should > use it, and if it is not, we should do the normal thing (a PATH search). > > This might require finding and fixing some places that set > DBUS_USE_TEST_BINARY but do not set DBUS_TEST_DAEMON, if there are any. Looks like there are none, but we need to introduce a parallel DBUS_TEST_DBUS_LAUNCH if we're going this route, and set it in the same places as DBUS_TEST_DAEMON.
Created attachment 121599 [details] [review] Replace $DBUS_USE_TEST_BINARY with $DBUS_TEST_DBUS_LAUNCH Instead of using $DBUS_USE_TEST_BINARY to control whether to use the hard-coded test binary TEST_BUS_LAUNCH_BINARY, we can just use $DBUS_TEST_DBUS_LAUNCH to control what we launch directly, as we were already doing for $DBUS_TEST_DAEMON.
Comment on attachment 119742 [details] [review] Windows _dbus_get_autolaunch_address: use DBUS_TEST_DATA etc. If this is how we're going to deal with test-autolaunch, it needs revising to not use DBUS_USE_TEST_BINARY, which has gone away. However, I think a better answer to test-autolaunch is to not run it on Windows at all, even if we're running the rest of name-test there, because of the unbounded process leak that I mentioned.
Comment on attachment 119735 [details] [review] Fix test-autolaunch test by generating session.conf in <build-dir>/share/dbus-1 for cmake. Am I right in thinking that the need for this patch goes away if we make sure to not run test-autolaunch on Windows?
Created attachment 121600 [details] [review] Replace $DBUS_USE_TEST_BINARY with $DBUS_TEST_DBUS_LAUNCH --- Same as Attachment #121599 [details], except progpath now needs to be const
(In reply to Simon McVittie from comment #33) > Comment on attachment 120166 [details] [review] [review] > Rename function string_array_length() to _dbus_string_array_length() and > move it to dbus-internals.c. > > Review of attachment 120166 [details] [review] [review]: > ----------------------------------------------------------------- > > While you're moving this around anyway, I'd prefer the empty body to be {} > rather than ; (to make it a little more visible). sure
(In reply to Simon McVittie from comment #34) > Comment on attachment 120170 [details] [review] [review] > Add new functions _dbus_string_array_replace() _dbus_string_array_append(). > > Review of attachment 120170 [details] [review] [review]: > ----------------------------------------------------------------- > > I'd be inclined to include (the replacement for) these in the same commit as > the Windows version of dbus-run-session - they're no use until that's added, > and when adding new APIs it's often helpful to see an example of their > real-world use. > > ::: dbus/dbus-internals.c > @@ +660,5 @@ > > + * > > + * @param array array to search. > > + * @param search string to search for. > > + * @returns replace string to replace. > > + * @returns TRUE if search string has been found, FALSE otherwise. > > If this returns FALSE, you can't tell whether that means "search string not > found" or "search string found, but then we ran out of memory". See below > for more comments on error-handling. > > @@ +668,5 @@ > > +{ > > + size_t i, len = _dbus_string_array_length (array); > > + for (i=0 ; i < len; i++) > > + { > > + if (strstr (array[i], search)) > > This searches for any string that *contains* @search, which is quite likely > to do the wrong thing. Pseudocode: > > _dbus_string_array_replace ({ "FOO=BAR", "BAZ=BADGER", NULL }, "BAD", > "BAD=LUCK") > would unexpectedly replace $BAZ with BAD=LUCK. Similarly, > > _dbus_string_array_replace ({ "FOO=23", "BARRACUDA=a big fish", NULL }, > "BAR", "BAR=42") > > would unexpectedly replace $BARRACUDA with BAR=42. This is simple a question of specifing good and uniq search terms, which could be documented. A working example would be _dbus_string_array_replace ({ "FOO=BAR", "BAZ=BADGER", NULL }, "BAZ=", "BAZ=LUCK") _dbus_string_array_replace ({ "FOO=23", "BARRACUDA=a big fish", NULL }, "FOO=", "BAR=42") > I think this would be better if these two new functions were combined into > one that looked more like g_environ_setenv(). > Please remember that dbus_string_array covers an array of common strings not dedicated <KEY>=<value> pairs as g_environ_setenv() provides. From my point of view a simple and straight api should support the following use cases: * contains(array, string) - check if a substring is somewhere present * size(array) - return size of array * append(array, string) - append string to array * remove(array, index) - remove string from array * search(array, string) - search string in array (a more general form of contains()) * replace(array, search, replace) - replace string in array by another (optional) extensions to the search use cases would be * starts_width(array, string) -> returns index, NOTFOUND, INVALID_ARRAY * ends_width(array, string) -> returns index, NOTFOUND, INVALID_ARRAY * search() with a kind of regular expression support The related functions to implement are all prefixed with _dbus_string_array_. > There are three valid signatures for a libdbus function that can fail: > The possible returns will be * ..contains(array, string) -> returns FOUND, NOTFOUND, INVALID_ARRAY * ..size(array) -> returns number of entries (0..n), INVALID_ARRAY * ..append(array, string) -> returns OKAY, INVALID_ARRAY, OOM * ..remove(array, index) -> returns OKAY, INVALID_ARRAY, INVALID_INDEX * ..search(array, string) -> returns index, NOTFOUND, INVALID_ARRAY * ..replace(array, search, replace) -> returns OKAY, NOTFOUND, INVALID_ARRAY, OOM where the error conditions could be defined as: #define OKAY 1 #define FOUND 1 #define NOTFOUND -1 #define INVALID_ARRAY -2 #define INVALID_INDEX -3 #define OOM -4 Required function for the recent bug are ..append() and ...replace()
(In reply to Ralf Habacker from comment #42) > * contains(array, string) - check if a substring is somewhere present > * size(array) - return size of array > * append(array, string) - append string to array > * remove(array, index) - remove string from array > * search(array, string) - search string in array (a more general form of contains()) > * replace(array, search, replace) - replace string in array by another I missed to add some important functions * init(array) * duplicate(array) * free(array)
(In reply to Ralf Habacker from comment #42) > This is simple a question of specifing good and uniq search terms, which > could be documented. Sorry, no, it isn't. "Search" is not the right operation for structured data like the environment. If you don't like g_environ_setenv(), the other way we could deal with the environment for dbus-run-session would be to have functions something like this: DBusHashTable *_dbus_envp_to_hash_table (char **environ); char **_dbus_envp_from_hash_table (DBusHashTable *table); which would do the splitting at "=", and then do the setenv()-like operations via _dbus_hash_table_insert_string(). We basically already have the code for this - populate_environment() and bus_activation_get_environment() in bus/activation.c - so you could move those functions to dbus-hash.c with only minor changes. The down side of this is that it involves a lot of copying via dbus_malloc(). > A working example would be > > _dbus_string_array_replace ({ "FOO=BAR", "BAZ=BADGER", NULL }, "BAZ=", > "BAZ=LUCK") OK, now call that same thing but with { "FOO=BAR", "FOOBARBAZ=BADGER", NULL } as the initial value? Expected result: "BAZ=LUCK" is appended as a third entry; actual result: FOOBARBAZ is replaced. strstr() is just not the right tool for this job. > Please remember that dbus_string_array covers an array of common strings not > dedicated <KEY>=<value> pairs as g_environ_setenv() provides. I don't think an array of strings with generic array-of-string operations is an appropriate data structure for manipulating the environment. Environment entries have internal structure (the key=value arrangement), and that internal structure matters. > From my point > of view a simple and straight api should support the following use cases I don't think we need a generic string-array-manipulation API. If we did, it should look more like GPtrArray or DBusString - an opaque or semi-opaque struct containing { int length, int length_allocated, char ** data } or similar. But for dbus-run-session's needs, I think we'd be better off with functions that specifically address an environ-like list of key/value pairs. Elsewhere in dbus, when we need a data structure containing multiple strings, it's normally a DBusList or DBusHashTable. As a general design principle, I don't want to introduce utility code into libdbus unless something immediately benefits from it. libdbus is not designed to be a general-purpose utility library: it should do what the tools in dbus need, and no more. We already have a reinvention of a reasonably large proportion of GLib, under a worse license - please don't make it larger!
Created attachment 121619 [details] [review] Add new functions _dbus_hash_table_to_array() and _dbus_hash_table_from_array() from related activation code.
Comment on attachment 119735 [details] [review] Fix test-autolaunch test by generating session.conf in <build-dir>/share/dbus-1 for cmake. This patch is obsolate in case dbus-daemon is never started with --session from the build dir.
Created attachment 121621 [details] [review] Rename function string_array_length() to _dbus_string_array_length() and move it to dbus-internals.c.
Created attachment 121623 [details] [review] Add windows implementation of dbus-run-session helper tool.
Comment on attachment 121623 [details] [review] Add windows implementation of dbus-run-session helper tool. Review of attachment 121623 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +3569,5 @@ > dbus_error_free (&error); > } > > +void > +_dbus_win_stderr_win_error (const char *app, const char *message, unsigned long code) I'd just have put this in dbus-run-session.c, but this is OK too, assuming "unsigned long" is the right type for a Windows error. ::: dbus/dbus-sysdeps-win.h @@ +96,5 @@ > > DBUS_PRIVATE_EXPORT > dbus_bool_t _dbus_getsid(char **sid, dbus_pid_t process_id); > + > +HANDLE _dbus_spawn_program (const char* name, char** argv, char** envp); I'm surprised this doesn't need a DBUS_PRIVATE_EXPORT? ::: tools/dbus-run-session.c @@ +41,2 @@ > #include "dbus/dbus.h" > +#include <dbus/dbus-spawn.h> Is this really the right header? I would have expected you to need <dbus/dbus-sysdeps-win.h> on Windows, now. @@ +350,5 @@ > + > + _dbus_string_init (&argv_strings[0]); > + _dbus_string_init (&argv_strings[1]); > + _dbus_string_init (&argv_strings[2]); > + _dbus_string_init (&argv_strings[3]); Sorry, I didn't spot this previously. If any of these fail, you should continue to init the others, but then fail with OOM (goto out). @@ +364,5 @@ > + dbus_daemon_argv[2] = _dbus_string_get_data (&argv_strings[2]); > + dbus_daemon_argv[3] = NULL; > + _DBUS_STATIC_ASSERT (_DBUS_N_ELEMENTS (dbus_daemon_argv) == 4); > + > + server_handle = _dbus_spawn_program (dbus_daemon, dbus_daemon_argv, 0); The "0" is still unexpected; it's a pointer, so NULL. @@ +382,5 @@ > + goto out; > + } > + dbus_free_string_array (env); > + > + if (!_dbus_hash_table_insert_string (env_table, "DBUS_SESSION_BUS_ADDRESS", _dbus_string_get_data (&argv_strings[3]))) Also please remove DBUS_STARTER_BUS_ADDRESS if present, like the Unix code path does. It can alter the bus that session services end up using (Bug #39196, Bug #63119). The other variables that are removed (setenv to NULL) on the Unix code path should ideally be removed too, although I suspect they don't really matter on Windows.
Comment on attachment 121619 [details] [review] Add new functions _dbus_hash_table_to_array() and _dbus_hash_table_from_array() from related activation code. Review of attachment 121619 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-hash.c @@ +1829,5 @@ > } > > +/** > + * Imports a string array into a hash table > + * The hash table needs to be initialized as string hash table. ... needs to be initialized with string keys, and dbus_free() as both key and value free-function. @@ +1833,5 @@ > + * The hash table needs to be initialized as string hash table. > + * > + * @param table the hash table > + * @param array the string array to import > + * @param delimiter the delimiter to separate key and value In practice I don't think we'll ever use a delimiter other than '=', and I'd slightly prefer the simpler signature, but this is OK. @@ +1835,5 @@ > + * @param table the hash table > + * @param array the string array to import > + * @param delimiter the delimiter to separate key and value > + * @return #TRUE on success. > + * @return #FALSE on failure. "... #FALSE if not enough memory" (see my previous comments about the 3 valid calling conventions) @@ +1847,5 @@ > + int i; > + dbus_bool_t retval = FALSE; > + > + if (array == NULL) > + return FALSE; This should be a _dbus_assert (array != NULL) (and the same for the hash table) @@ +1899,5 @@ > +/** > + * Creates a string array from a hash table > + * > + * @param table the hash table > + * @param delimiter the delimiter to join key and value Again, I don't think we're likely to use this with a delimiter that isn't "=" in practice, but this is OK. @@ +1900,5 @@ > + * Creates a string array from a hash table > + * > + * @param table the hash table > + * @param delimiter the delimiter to join key and value > + * @return pointer to created string array ... (free with dbus_free_string_array) @@ +1901,5 @@ > + * > + * @param table the hash table > + * @param delimiter the delimiter to join key and value > + * @return pointer to created string array > + * @return #FALSE on failure. ... #FALSE if not enough memory
Comment on attachment 121621 [details] [review] Rename function string_array_length() to _dbus_string_array_length() and move it to dbus-internals.c. Review of attachment 121621 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-internals.c @@ +652,5 @@ > +{ > + size_t i; > + for (i = 0; array[i]; i++) > + { > + ; No need for this semicolon now, and some compilers will warn on it. "{}" is a valid do-nothing block, but strictly speaking, "{ ; }" is not valid ISO C.
Created attachment 121625 [details] [review] name-test C tests: produce structured (TAP) output Similar to commit 58eefa1031e14cb402ed0aae85e6bce1ba030a28. test-privserver is a helper executable, not a test. I moved its output from stdout to stderr so it can't be misinterpreted as the test's stdout. --- This is a step towards making the way we run these tests in Autotools more like the way you want to run them in CMake. Yes, "Bail out! ..." is really part of TAP syntax. Blame the Perl developers :-)
Created attachment 121626 [details] [review] name-test: run most C tests directly, not via run-test.sh The exception is test-autolaunch, which is really not particularly useful as a build-time test. The only way we can really test autolaunch is as a whole-system integration test, and "make check" is not that. The two tests written in Python and one test based on dbus-send are also not run directly yet; in particular, that includes both the tests in run-test-systemserver.sh. --- This still only runs those tests on Unix, but with a finished version of dbus-run-session.exe, I think we should in principle be able to reduce the amount of "if DBUS_UNIX", and run all the TESTS except run-test.sh and run-test-systemserver.sh on mingw builds too.
Created attachment 121630 [details] [review] Rename function string_array_length() to _dbus_string_array_length() and move it to dbus-internals.c.
Created attachment 121631 [details] [review] Add windows implementation of dbus-run-session helper tool. (update 2)
(In reply to Simon McVittie from comment #49) > Comment on attachment 121623 [details] [review] [review] > Add windows implementation of dbus-run-session helper tool. > > Review of attachment 121623 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-sysdeps-win.c > @@ +3569,5 @@ > > dbus_error_free (&error); > > } > > > > +void > > +_dbus_win_stderr_win_error (const char *app, const char *message, unsigned long code) > > I'd just have put this in dbus-run-session.c, but this is OK too, assuming > "unsigned long" is the right type for a Windows error. yes, see https://msdn.microsoft.com/en-us/library/cc230318.aspx typedef unsigned long DWORD, *PDWORD, *LPDWORD; > > ::: dbus/dbus-sysdeps-win.h > @@ +96,5 @@ > > > > DBUS_PRIVATE_EXPORT > > dbus_bool_t _dbus_getsid(char **sid, dbus_pid_t process_id); > > + > > +HANDLE _dbus_spawn_program (const char* name, char** argv, char** envp); > > I'm surprised this doesn't need a DBUS_PRIVATE_EXPORT? this is located in dbus-internal > > ::: tools/dbus-run-session.c > @@ +41,2 @@ > > #include "dbus/dbus.h" > > +#include <dbus/dbus-spawn.h> > > Is this really the right header? I would have expected you to need > <dbus/dbus-sysdeps-win.h> on Windows, now. > fixed > @@ +350,5 @@ > > + > > + _dbus_string_init (&argv_strings[0]); > > + _dbus_string_init (&argv_strings[1]); > > + _dbus_string_init (&argv_strings[2]); > > + _dbus_string_init (&argv_strings[3]); > > Sorry, I didn't spot this previously. If any of these fail, you should > continue to init the others, but then fail with OOM (goto out). fixed with update 3 > @@ +364,5 @@ > > + dbus_daemon_argv[2] = _dbus_string_get_data (&argv_strings[2]); > > + dbus_daemon_argv[3] = NULL; > > + _DBUS_STATIC_ASSERT (_DBUS_N_ELEMENTS (dbus_daemon_argv) == 4); > > + > > + server_handle = _dbus_spawn_program (dbus_daemon, dbus_daemon_argv, 0); > > The "0" is still unexpected; it's a pointer, so NULL. fixed > @@ +382,5 @@ > > + goto out; > > + } > > + dbus_free_string_array (env); > > + > > + if (!_dbus_hash_table_insert_string (env_table, "DBUS_SESSION_BUS_ADDRESS", _dbus_string_get_data (&argv_strings[3]))) > > Also please remove DBUS_STARTER_BUS_ADDRESS if present, like the Unix code > path does. It can alter the bus that session services end up using (Bug > #39196, Bug #63119). removed DBUS_STARTER_ADDRESS > The other variables that are removed (setenv to NULL) on the Unix code path > should ideally be removed too, although I suspect they don't really matter > on Windows. removed DBUS_STARTER_BUS_TYPE
Created attachment 121632 [details] [review] Add windows implementation of dbus-run-session helper tool. (update 3)
Comment on attachment 121600 [details] [review] Replace $DBUS_USE_TEST_BINARY with $DBUS_TEST_DBUS_LAUNCH Review of attachment 121600 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 121625 [details] [review] name-test C tests: produce structured (TAP) output Review of attachment 121625 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 121626 [details] [review] name-test: run most C tests directly, not via run-test.sh Review of attachment 121626 [details] [review]: ----------------------------------------------------------------- As this stuff is unix only it looks good. On windows the requirements to run test-autolaunch are much lower because of the usage of relative paths in session.conf. It is only required to have a <build-dir>/share/dbus-1/session.conf, the legacy conf in <build-dir>/etc/dbus-1/session.conf and <builddir>/share/dbus-1/services.
Created attachment 121635 [details] [review] Add new functions _dbus_hash_table_to_array() and _dbus_hash_table_from_array() from related activation code (update) (In reply to Simon McVittie from comment #50) > Comment on attachment 121619 [details] [review] [review] > Add new functions _dbus_hash_table_to_array() and > _dbus_hash_table_from_array() from related activation code. > > Review of attachment 121619 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-hash.c > @@ +1829,5 @@ > > } > > > > +/** > > + * Imports a string array into a hash table > > + * The hash table needs to be initialized as string hash table. > > ... needs to be initialized with string keys, and dbus_free() as both key > and value free-function. fixed > > @@ +1833,5 @@ > > + * The hash table needs to be initialized as string hash table. > > + * > > + * @param table the hash table > > + * @param array the string array to import > > + * @param delimiter the delimiter to separate key and value > > In practice I don't think we'll ever use a delimiter other than '=', and I'd > slightly prefer the simpler signature, but this is OK. > > @@ +1835,5 @@ > > + * @param table the hash table > > + * @param array the string array to import > > + * @param delimiter the delimiter to separate key and value > > + * @return #TRUE on success. > > + * @return #FALSE on failure. > > "... #FALSE if not enough memory" (see my previous comments about the 3 > valid calling conventions) fixed > @@ +1847,5 @@ > > + int i; > > + dbus_bool_t retval = FALSE; > > + > > + if (array == NULL) > > + return FALSE; > > This should be a _dbus_assert (array != NULL) (and the same for the hash > table) added > > @@ +1899,5 @@ > > +/** > > + * Creates a string array from a hash table > > + * > > + * @param table the hash table > > + * @param delimiter the delimiter to join key and value > > Again, I don't think we're likely to use this with a delimiter that isn't > "=" in practice, but this is OK. > > @@ +1900,5 @@ > > + * Creates a string array from a hash table > > + * > > + * @param table the hash table > > + * @param delimiter the delimiter to join key and value > > + * @return pointer to created string array > > ... (free with dbus_free_string_array) > > @@ +1901,5 @@ > > + * > > + * @param table the hash table > > + * @param delimiter the delimiter to join key and value > > + * @return pointer to created string array > > + * @return #FALSE on failure. > > ... #FALSE if not enough memory fixed
Comment on attachment 121626 [details] [review] name-test: run most C tests directly, not via run-test.sh committed to dbus-1.10
Comment on attachment 121630 [details] [review] Rename function string_array_length() to _dbus_string_array_length() and move it to dbus-internals.c. committed to dbus-1.10
Comment on attachment 121600 [details] [review] Replace $DBUS_USE_TEST_BINARY with $DBUS_TEST_DBUS_LAUNCH committed to dbus-1.10
Comment on attachment 121625 [details] [review] name-test C tests: produce structured (TAP) output committed to dbus-1.10
Comment on attachment 120170 [details] [review] Add new functions _dbus_string_array_replace() _dbus_string_array_append(). This patch is now unrelated and may be added if useful with another bug ?
Comment on attachment 121635 [details] [review] Add new functions _dbus_hash_table_to_array() and _dbus_hash_table_from_array() from related activation code (update) Review of attachment 121635 [details] [review]: ----------------------------------------------------------------- This looks good, but unfortunately the code you're moving has memory leaks in some of the OOM code paths (as usual, all error-recovery that isn't tested is wrong). Committing it as-is is fine, but we should also fix the leaks while we're looking at this. ::: dbus/dbus-hash.c @@ +1874,5 @@ > + if (!_dbus_string_steal_data (&key, &hash_key)) > + break; > + > + if (!_dbus_string_steal_data (&value, &hash_value)) > + break; Pre-existing bug: this leaks hash_key. @@ +1878,5 @@ > + break; > + > + if (!_dbus_hash_table_insert_string (table, > + hash_key, hash_value)) > + break; Pre-existing bug: this leaks hash_value. @@ +1879,5 @@ > + > + if (!_dbus_hash_table_insert_string (table, > + hash_key, hash_value)) > + break; > + } Pre-existing bug: this doesn't distinguish between "delimiter was not found" and "not enough memory". It should probably be something like this: char delimiter_str[] = { delimiter, '\0' }; if (_dbus_string_split_on_byte (&key, delimiter, &value)) { ... do what it does now ... } else if (_dbus_string_find (&key, 0, delimiter_str, NULL)) { /* out of memory */ break; } /* else ignore entry with no delimiter */ If delimiter was hard-coded to '=' instead of being a parameter, we wouldn't need delimiter_str and could just use "=". In practice I don't think we'll ever call this with a different delimiter, and if I'm wrong we can just put the parameter back in, so I would be inclined to remove the parameter.
Comment on attachment 121632 [details] [review] Add windows implementation of dbus-run-session helper tool. (update 3) Review of attachment 121632 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-hash.c @@ +340,5 @@ > + * Destroy a hash table > + * @param table the hash table to destroy > + */ > +dbus_bool_t > +_dbus_hash_table_free (DBusHashTable *table) I'd prefer to leave this freeing code where it is, and use the existing unref in dbus-run-session. @@ +343,5 @@ > +dbus_bool_t > +_dbus_hash_table_free (DBusHashTable *table) > +{ > + if (table == NULL) > + return FALSE; The GLib convention (which libdbus follows) is that opaque objects' free() or unref() functions are not valid to call with a NULL argument, and the caller should do the NULL check. This means strings and malloc'd arrays of malloc'd strings (aka GStrv in GLib jargon) are freed with a version of whatever_free() that is OK to call on NULL, but for DBusHashTable, GPtrArray, etc. it is invalid to call the free- or unref-function on a NULL argument. Please stick to that convention: it's more important to be predictable than to save a couple of lines of "if (foo != NULL) thing_free (foo)". (The reasoning is that NULL is not a DBusHashTable - at best it's the absence of a DBusHashTable.) ::: dbus/dbus-sysdeps-win.h @@ +44,4 @@ > const char* _dbus_win_error_from_last_error (void); > > dbus_bool_t _dbus_win_startup_winsock (void); > +DBUS_PRIVATE_EXPORT Is this still needed? ::: tools/dbus-run-session.c @@ +428,5 @@ > + _dbus_string_free (&argv_strings[1]); > + _dbus_string_free (&argv_strings[2]); > + _dbus_string_free (&argv_strings[3]); > + dbus_free_string_array (env); > + _dbus_hash_table_free (env_table); Just use if (env_table != NULL) _dbus_hash_table_unref (env_table);
(In reply to Ralf Habacker from comment #66) > This patch is now unrelated and may be added if useful with another bug ? If it becomes useful, we can consider adding it, but we shouldn't do that until/unless something wants to call it. (It seems fairly unlikely that the semantics of this version of _dbus_string_array_replace() are something we'll ever want in practice, but I could be wrong...)
Comment on attachment 121635 [details] [review] Add new functions _dbus_hash_table_to_array() and _dbus_hash_table_from_array() from related activation code (update) (In reply to Simon McVittie from comment #67) > Comment on attachment 121635 [details] [review] [review] > Add new functions _dbus_hash_table_to_array() and > _dbus_hash_table_from_array() from related activation code (update) > > Review of attachment 121635 [details] [review] [review]: > ----------------------------------------------------------------- > > This looks good, but unfortunately the code you're moving has memory leaks > in some of the OOM code paths (as usual, all error-recovery that isn't > tested is wrong). > > Committing it as-is is fine committed to dbus-1.10
(In reply to Simon McVittie from comment #67) > but we should also fix the leaks while we're looking at this. > > ::: dbus/dbus-hash.c > @@ +1874,5 @@ > > + if (!_dbus_string_steal_data (&key, &hash_key)) > > + break; _dbus_string_steal_data() looks wired [1] { DBUS_STRING_PREAMBLE (str); _dbus_assert (data_return != NULL); undo_alignment (real); *data_return = (char*) real->str; /* reset the string */ if (!_dbus_string_init (str)) { /* hrm, put it back then */ real->str = (unsigned char*) *data_return; *data_return = NULL; fixup_alignment (real); return FALSE; } return TRUE; } It changes the variable pointed by data_return and reset it to another value in case something goes wrong in the inside, which means the return value has been changed although nothing has been returned. Not better to save real->str into a local variable first and only setup data_return in case of no errors ? { char *s; DBUS_STRING_PREAMBLE (str); _dbus_assert (data_return != NULL); undo_alignment (real); s = (char*) real->str; /* reset the string */ if (!_dbus_string_init (str)) { /* hrm, put it back then */ real->str = (unsigned char*) s; fixup_alignment (real); return FALSE; } *data_return = s; return TRUE; } > > + if (!_dbus_string_steal_data (&key, &hash_key)) > > + break; > Pre-existing bug: this leaks hash_key. In the return FALSE case, according the the implementation [1], hash_key is set to zero, so no leak here. It is simply required to make sure, that key is free'd, which is the case > > @@ +1878,5 @@ > > + break; > > + > > + if (!_dbus_hash_table_insert_string (table, > > + hash_key, hash_value)) > > + break; > > Pre-existing bug: this leaks hash_value. dito
(In reply to Simon McVittie from comment #44) > As a general design principle, I don't want to introduce utility code into > libdbus unless something immediately benefits from it. libdbus is not designed to be a general-purpose utility library: it should do what the tools in dbus need, and no more. But having a basic set of function decreases the learning curve. In the recent case I (and probably other people in different areas before) needed to spent much time to deal with the missing dbus_string_array functions instead concentrating on the windows port of dbus-run-session. Writing additional functions in the right way is not a trivial task. Also the api should be consistent e.g. we have _dbus_string_free(), _dbus_rlimit_free(), _dbus_error_free () and more, so I expect to have dbus_string_array_free(), but it is named dbus_free_string_array(), which needs more time to find for non every day dbus programmer. Also we have dbus_new0 () and the counterpart dbus_free(), dbus_string_init() and dbus_string_free(), _dbus_data_slot_allocator_init() and _dbus_data_slot_list_free() and others without reference counting. But bus_policy_new() and others supporting reference counting (which implicit includes calling bus_policy_ref()) and bus_policy_unref(), which implicit includes bus_policy_free (), but bus_policy_free () is not available. In the dbus-run-session case I do not care about reference counting, I simply want to create a new hash table, use it, and want to delete it later. For that ..._new() and ..._free() functions would be easy to use and fits the design. > We already have a reinvention of a reasonably large proportion of GLib, > under a worse license - please don't make it larger! Did this happened because glib does not cover the required dbus needs ? As far as I can see is glib not a part of dbus core, only used in test case. I guess dbus has been started without glib and glib has later been added because is it easier to use a framework as to rewrite everything from scratch. (At least I wrote some tests with Qt because it is much faster).
(In reply to Ralf Habacker from comment #70) > > Committing it as-is is fine > > committed to dbus-1.10 Er, what? No. Moving functions around that don't need to be moved around is not a stable-branch change. The rule for stable-branch changes is roughly "could I justify this to the Debian release team as something that needs to be pushed out to all users of stable?", and this doesn't meet that. It's fine for master, but I'm going to revert it on dbus-1.10.
(In reply to Simon McVittie from comment #73) > (In reply to Ralf Habacker from comment #70) > > > Committing it as-is is fine > > > > committed to dbus-1.10 > > Er, what? No. Moving functions around that don't need to be moved around is > not a stable-branch change. The rule for stable-branch changes is roughly > "could I justify this to the Debian release team as something that needs to > be pushed out to all users of stable?", and this doesn't meet that. > > It's fine for master, but I'm going to revert it on dbus-1.10. The bug version is set to dbus-1.10.
(In reply to Ralf Habacker from comment #71) > _dbus_string_steal_data() looks wired ... > It changes the variable pointed by data_return and reset it to another value > in case something goes wrong in the inside, which means the return value has > been changed although nothing has been returned. It does look a bit weird, but arguably it has "returned NULL"... > > > + if (!_dbus_string_steal_data (&key, &hash_key)) > > > + break; > > Pre-existing bug: this leaks hash_key. > > In the return FALSE case, according the the implementation [1], hash_key is > set to zero, so no leak here. Sorry, Splinter's choice of what to quote was misleading. What I meant is: > if (!_dbus_string_steal_data (&key, &hash_key)) > break; If this fails, that's fine: we don't leak. If this succeeds, then we have truncated @key, and taken responsibility for either freeing @hash_key or putting it in the hash table where it will be freed later. > if (!_dbus_string_steal_data (&value, &hash_value)) > break; If this fails, we don't leak hash_value, but we are still responsible for freeing hash_key, and we don't. If this succeeds, we have truncated @value, and we are now responsible for either freeing both @hash_key and @hash_value or putting them in the hash table. > if (!_dbus_hash_table_insert_string (activation->environment, > hash_key, hash_value)) > break; If this fails, then hash_key and hash_value are leaked. If this succeeds, then everything is fine: we've handed over responsibility for freeing them to the hash table, and we no longer own those pointers.
(In reply to Ralf Habacker from comment #72) > > As a general design principle, I don't want to introduce utility code into > > libdbus unless something immediately benefits from it. ... > In the recent case I (and probably other people in different areas before) > needed to spent much time to deal with the missing dbus_string_array > functions instead concentrating on the windows port of dbus-run-session. > Writing additional functions in the right way is not a trivial task. This is, unfortunately, a natural consequence of libdbus not having external dependencies. If it was technically and politically acceptable to depend on GLib, I'd do that, but it isn't :-( There's no point in investing the time to add utility functions until we'd actually be using them: if you do that, you've spent some time but not actually improved libdbus, and we have more code to maintain in future. If you only add what we actually need when we need it, you'll have spent no more time on it overall, and the library will be smaller. > Also the api should be consistent e.g. we have _dbus_string_free(), > _dbus_rlimit_free(), _dbus_error_free () and more, so I expect to have > dbus_string_array_free(), but it is named dbus_free_string_array(), which > needs more time to find for non every day dbus programmer. I didn't name that function, but I think the difference is that _dbus_everything_else_free() frees an opaque data structure designed by libdbus, whereas dbus_free_string_array() frees a non-opaque "plain C" data structure (an environ-style NULL-terminated string array) which in general we avoid. We can't rename dbus_free_string_array() in any case, because it's public API. For non-every-day D-Bus programmers, I would strongly recommend GDBus, QtDBus or sd-bus; they're better libraries. libdbus is the support library for dbus-daemon: that gives it some API requirements that the others don't have, like coping with malloc() returning NULL. > Also we have > dbus_new0 () and the counterpart dbus_free(), > dbus_string_init() and dbus_string_free(), > _dbus_data_slot_allocator_init() and _dbus_data_slot_list_free() and others > without reference counting. Right, not everything is refcounted. This is just a fact of C API design: you can't put a refcount in a char **. For the data slot allocator etc., not being refcounted is presumably an optimization. It might reasonably be considered premature optimization, and I'd encourage use of a refcount in new data structures (unless we're allocating enough of them that it matters). Any given data structure should either be refcounted, or not - a mixture of the two just leads to weird bugs. > > We already have a reinvention of a reasonably large proportion of GLib, > > under a worse license - please don't make it larger! > > Did this happened because glib does not cover the required dbus needs ? Yes. GLib aborts if malloc() returns NULL, which is considered unacceptable for dbus-daemon. Unfortunately, this means that everything that uses libdbus pays the complexity price for coping with inability to allocate memory, even in contexts where it will literally never happen (Linux with the default overcommit behaviour) or in applications where crashing on out-of-memory is acceptable (if a GUI app runs out of memory, it won't have enough memory to do anything about it, so it has little alternative). I think it was also partly political, because Havoc wanted wide adoption of dbus-daemon even in environments where GLib isn't normally installed. I've been introducing GLib into non-essential bits (i.e. tests) because life's too short to rewrite GLib's test framework under a worse license. > because is it easier to use a framework as to rewrite everything from > scratch. (At least I wrote some tests with Qt because it is much faster). Yes, this is a similar idea. However, as well as being considerably "heavier" than GLib and written in a different language, Qt depends on dbus (for QtDBus), so using it for tests inside libdbus would be a circular dependency. We had a similar problem with dbus and dbus-glib, which we managed to fix about a year ago; and we still have a circular dependency on dbus-python, although that only affects a couple of tests, which can be disabled. Maybe one day I'll rewrite those tests in C. For general things related to D-Bus (the protocol), QtDBus is a fine choice, although I would personally recommend GDBus over it. For dbus (the reference implementation of D-Bus), we shouldn't use it.
(In reply to Ralf Habacker from comment #74) > (In reply to Simon McVittie from comment #73) > > It's fine for master, but I'm going to revert it on dbus-1.10. > > The bug version is set to dbus-1.10. I've generally interpreted the bug version on this Bugzilla as "where the bug was found". The original bug you reported was that test-autolaunch.exe fails, which is indeed a true fact about dbus-1.10. Some Bugzilla installations have separate fields for the version where the bug was found and the version where it is intended to be fixed, but unfortunately ours doesn't. Regardless of bug versioning, only non-intrusive fixes for user-visible bugs should go to our stable branches: if our stable branches aren't suitable for use by long-term-support OS distributions, then there's no point in maintaining them. I've reverted 5 test-related patches from dbus-1.10, and cherry-picked them onto master instead.
-- 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/135.
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.