At Bug 92538 Comment 22 there has been reported that running make check on a cross compile build for windows returns the following errors: sed -e 's![@]RUN[@]!../bus/test-bus.exe!' \ < ../../dbus-1/test/tap-test.sh.in > test-bus.sh FAIL: test-bus.sh 1 ../bus/test-bus.exe (exit status 1) and sed -e 's![@]RUN[@]!../dbus/test-dbus.exe!' \ < ../../dbus-1/test/tap-test.sh.in > test-dbus.sh FAIL: test-dbus.sh 1 ../dbus/test-dbus.exe (exit status 3)
(In reply to Ralf Habacker from comment #0) > sed -e 's![@]RUN[@]!../bus/test-bus.exe!' \ > < ../../dbus-1/test/tap-test.sh.in > test-bus.sh > FAIL: test-bus.sh 1 ../bus/test-bus.exe (exit status 1) > S:/xxx/src/dbus-1-autotools-cross-build/bus/.libs/test-bus.exe: Running config file parser test Testing retrieving the default session service directories default service dir: /usr/local/share/dbus-1/services default service dir: C:\Program Files\Common Files/dbus-1/services test service dir: /usr/local/share/dbus-1/services /usr/local/share/dbus-1/services directory does not match S:\xxx\src\dbus-1-autotools-cross-build\dbus\.libs\/usr/local/share/dbus-1/services in the match set Unit test failed: parser
You're the Windows expert: which is correct? The "expected" result from the test, the actual result from the test, or neither?
(In reply to Simon McVittie from comment #2) > You're the Windows expert: which is correct? The "expected" result from the > test, the actual result from the test, or neither? Because we are cross compiling local host pathes should not be used as default service dir.
(In reply to Ralf Habacker from comment #1) > (In reply to Ralf Habacker from comment #0) > > > sed -e 's![@]RUN[@]!../bus/test-bus.exe!' \ > > < ../../dbus-1/test/tap-test.sh.in > test-bus.sh > > FAIL: test-bus.sh 1 ../bus/test-bus.exe (exit status 1) > > > > S:/xxx/src/dbus-1-autotools-cross-build/bus/.libs/test-bus.exe: Running > config file parser test > Testing retrieving the default session service directories > default service dir: /usr/local/share/dbus-1/services > default service dir: C:\Program Files\Common Files/dbus-1/services > test service dir: /usr/local/share/dbus-1/services > /usr/local/share/dbus-1/services directory does not match > S:\xxx\src\dbus-1-autotools-cross-build\dbus\.libs\/usr/local/share/dbus-1/ > services in the match set > Unit test failed: parser here is a variant generated from running with a cmake build: 2: Test command: /home/xxx/src/dbus-cmake-cross-x86-build/bin/test-bus.exe 2: Environment variables: 2: DBUS_TEST_DAEMON=z:/home/xxx/src/dbus-cmake-cross-x86-build/bin/dbus-daemon.exe 2: DBUS_SESSION_BUS_ADDRESS= 2: DBUS_FATAL_WARNINGS=1 2: DBUS_TEST_DATA=z:/home/xxx/src/dbus-cmake-cross-x86-build/test/data 2: DBUS_TEST_HOMEDIR=z:/home/xxx/src/dbus-cmake-cross-x86-build/dbus 2: Test timeout computed to be: 9.99988e+06 2: S:\xxx\src\dbus-cmake-cross-x86-build\bin\test-bus.exe: Running expire list test 2: S:\xxx\src\dbus-cmake-cross-x86-build\bin\test-bus.exe: checking for memleaks 2: S:\xxx\src\dbus-cmake-cross-x86-build\bin\test-bus.exe: Running config file parser test 2: Testing retrieving the default session service directories 2: default service dir: S:/xxx/src/dbus-cmake-cross-x86-build/share/dbus-1/services 2: default service dir: C:\Program Files\Common Files/dbus-1/services 2: test service dir: S:/xxx/src/dbus-cmake-cross-x86-build/share/dbus-1/services 2: S:/xxx/src/dbus-cmake-cross-x86-build/share/dbus-1/services directory does not match S:/xxx/src/dbus-cmake-cross-x86-build/share in the match set 2: Unit test failed: parser
Created attachment 119290 [details] [review] Fix runtime path of DBUS_DATADIR.
(In reply to Ralf Habacker from comment #5) > Created attachment 119290 [details] [review] [review] > Fix runtime path of DBUS_DATADIR. With that patch I get with cmake build: 2: Testing retrieving the default session service directories 2: default service dir: S:/xxx/src/dbus-1-cmake-cross-x86-build/share/dbus-1/services 2: default service dir: C:\Program Files\Common Files/dbus-1/services 2: test service dir: 'S:/xxx/src/dbus-1-cmake-cross-x86-build/share/dbus-1/services' 2: 'S:/xxx/src/dbus-1-cmake-cross-x86-build/share/dbus-1/services' directory does not match 'S:/xxx/src/dbus-1-cmake-cross-x86-build/share/dbus-1/services' in the match set
Created attachment 119291 [details] [review] Wrap path verbose output with '' to be able to see trailing spaces. With this patch applied there are no trailing spaces in the path to see and the default service dir and test service looks equal in term of character case, but the match still fails. Then I printed out the length of both strings and got: 2: length test service dir: 62 2: length standard service dir: 63
Created attachment 119292 [details] [review] Fix runtime path of DBUS_DATADIR (update 1) fixed initialisation of buffer
(In reply to Ralf Habacker from comment #7) length of both strings and got: > > 2: length test service dir: 62 > 2: length standard service dir: 63 fixed by updated patch. The remaining issue on running bus-test is autotools: Unknown username "root" in message bus configuration file cmake: 2: Unknown username "root" on element <deny>
(In reply to Ralf Habacker from comment #9) > (In reply to Ralf Habacker from comment #7) > length of both strings and got: > > > > 2: length test service dir: 62 > > 2: length standard service dir: 63 > > fixed by updated patch. > > The remaining issue on running bus-test is > > autotools: > Unknown username "root" in message bus configuration file > > cmake: > 2: Unknown username "root" on element <deny> These are the relevant files: test/data/valid-config-files/many-rules.conf:20: <deny user="root"/> test/data/valid-config-files/many-rules.conf:39: <deny user="root"/> which need to be setup on configure time. If noone objects I would add build system variables DBUS_TEST_ROOT_USER and DBUS_TEST_ROOT_USER for that purpose. Additional the related config parser need to be extended for windows.
Created attachment 119312 [details] [review] Fix bus-test parser check error caused by using wrong runtime path of DBUS_DATADIR. Add missing prototype.
Created attachment 119313 [details] [review] Fix bus-test error 'Unknown username "root" on element <deny>'
(In reply to Ralf Habacker from comment #12) > Created attachment 119313 [details] [review] [review] > Fix bus-test error 'Unknown username "root" on element <deny>' After applying this patch the next displayed error message is "Expecting NameAcquired, got nothing". See appended file error.log for details
Created attachment 119316 [details] error.log
Created attachment 119317 [details] [review] Fix bus-test error 'Unknown username "root" on element <deny>' (update 1) remove obsolate debug message
Comment on attachment 119291 [details] [review] Wrap path verbose output with '' to be able to see trailing spaces. Review of attachment 119291 [details] [review]: ----------------------------------------------------------------- OK for master, too much noise for 1.10 IMO
Comment on attachment 119312 [details] [review] Fix bus-test parser check error caused by using wrong runtime path of DBUS_DATADIR. Review of attachment 119312 [details] [review]: ----------------------------------------------------------------- ::: bus/config-parser.c @@ +3407,5 @@ > const char *common_progs; > char buffer[1024]; > > + strcpy(buffer, _dbus_windows_get_datadir ()); > + strcat(buffer, "/dbus-1/services"); Can we please not have buffer overflows in new code? I won't insist on replacing this buffer handling with DBusString (although that would be the fully-correct answer), but strncpy() and strncat() would be an easy improvement.
Created attachment 119341 [details] [review] Fix bus-test parser check error caused by using wrong runtime path of DBUS_DATADIR (update 1)
Comment on attachment 119317 [details] [review] Fix bus-test error 'Unknown username "root" on element <deny>' (update 1) Review of attachment 119317 [details] [review]: ----------------------------------------------------------------- This is a lot of code for something we'll probably never use. The well-known system bus is not supported on Windows, and the well-known session bus doesn't need multi-user support. I'd prefer to copy many-rules.conf into valid-config-files-system/, test that directory on Unix only, and remove the <user> and <group> rules from valid-config-files/many-rules.conf for use on Windows. I'll attach a patch for that. (I haven't reviewed in detail.)
Comment on attachment 119291 [details] [review] Wrap path verbose output with '' to be able to see trailing spaces. committed to master
Comment on attachment 119341 [details] [review] Fix bus-test parser check error caused by using wrong runtime path of DBUS_DATADIR (update 1) Review of attachment 119341 [details] [review]: ----------------------------------------------------------------- ::: bus/config-parser.c @@ +3407,4 @@ > const char *common_progs; > char buffer[1024]; > > + strncpy(buffer, _dbus_windows_get_datadir (), sizeof (buffer)); Also needs buffer[1023] = '\0' (strncpy does not guarantee to terminate if truncation occurs) @@ +3407,5 @@ > const char *common_progs; > char buffer[1024]; > > + strncpy(buffer, _dbus_windows_get_datadir (), sizeof (buffer)); > + strncat(buffer, "/dbus-1/services", sizeof (buffer)); Needs to be sizeof (buffer) - strlen (buffer) - 1, if I've got my pointer arithmetic right. Using a DBusString might be safer: DBusString buffer; if (!_dbus_string_init (&buffer) || !_dbus_string_append (&buffer, _dbus_windows_get_datadir ()) || !_dbus_string_append (&buffer, "/dbus-1/services"))) /* error: out of memory */ On master, I think we'll need a DBusString anyway, because _dbus_replace_install_prefix() changed its signature.
(In reply to Simon McVittie from comment #21) > Using a DBusString might be safer: > > DBusString buffer; > > if (!_dbus_string_init (&buffer) || > !_dbus_string_append (&buffer, _dbus_windows_get_datadir ()) || > !_dbus_string_append (&buffer, "/dbus-1/services"))) > /* error: out of memory */ > > On master, I think we'll need a DBusString anyway, because > _dbus_replace_install_prefix() changed its signature. Unfortunally introducing DBusString at this place requires to add many calls to _dbus_string_free() on any exit below this location.
Created attachment 119344 [details] [review] Test system bus config files on Unix only Previously, we didn't consistently test parsing of every file in valid-config-files-system/ everywhere that we tested valid-config-files/. We now test it on Unix. The system bus is not supported on Windows, so we do not test valid-config-files-system/ there. valid-config-files/many-rules.conf contains <user> and <group> rules which are not applicable to Windows. Copy the original many-rules.conf to valid-config-files-system/ so that it will be tested on Unix, and remove the non-portable rules from valid-config-files/many-rules.conf. --- I'd prefer this, rather than Attachment #119317 [details].
Created attachment 119345 [details] [review] test_default_session_servicedirs: simplify to a single exit code-path A similar simplification was already done on master as part of commit f830e14, Bug #83539.
Created attachment 119346 [details] [review] test_default_session_servicedirs: use the intended data directory If D-Bus was configured for /usr/local and built in Z:/build, the previous code would use Z:/build/dbus/.libs/usr/local/share/dbus-1/services whereas the intention was to replace the configured prefix /usr/local with the detected location, more like Z:/build/dbus/.libs/share/dbus-1/services
(In reply to Ralf Habacker from comment #22) > Unfortunally introducing DBusString at this place requires to add many calls > to _dbus_string_free() on any exit below this location. You're right that this would be really annoying without Attachment #119345 [details], but that's why the "goto out" idiom is useful. It's essentially try/finally, but in C; I've even used "goto finally" in other C projects, but "goto out" is the conventional name for this in dbus.
Created attachment 119347 [details] [review] Don't use _dbus_warn() for intentionally-skipped tests The tests are run with _dbus_warn() fatal, so if a particular test is not applicable on the current platform, we shouldn't call it. --- With all these patches applied to dbus-1.10, test-dbus can pass in a cross-build "make check" (testing with Debian, Autotools, mingw-w64, Wine 1.6.2). test-bus still fails, with the error you previously described: Z:/srv/tmp/smcv/build/dbus/1.10/mingw/bus/.libs/test-bus.exe: Running SHA1 connection test^M Expecting NameAcquired, got nothing^M I don't know what's going on there.
Comment on attachment 119344 [details] [review] Test system bus config files on Unix only Review of attachment 119344 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 119345 [details] [review] test_default_session_servicedirs: simplify to a single exit code-path Review of attachment 119345 [details] [review]: ----------------------------------------------------------------- looks good
(In reply to Simon McVittie from comment #27) > Created attachment 119347 [details] [review] [review] > Don't use _dbus_warn() for intentionally-skipped tests > > The tests are run with _dbus_warn() fatal, so if a particular test is > not applicable on the current platform, we shouldn't call it. > > --- > > With all these patches applied to dbus-1.10, test-dbus can pass in a > cross-build "make check" (testing with Debian, Autotools, mingw-w64, Wine > 1.6.2). with cmake test-dbus passed also :-) 1/1 Test #1: test-dbus ........................ Passed 74.83 sec > > test-bus still fails, with the error you previously described: still have the unknown username "root" failure with cmake 2: 987: [dbus/dbus-file-win.c(134):_dbus_file_get_contents] file z:/home/ralf.habacker/src/dbus-1-cmake-cross-x86-build/test/data\valid-config-files\system.conf hnd 0000008C opened 2: Unknown username "root" in message bus configuration file > > Z:/srv/tmp/smcv/build/dbus/1.10/mingw/bus/.libs/test-bus.exe: Running SHA1 > connection test^M > Expecting NameAcquired, got nothing^M > > I don't know what's going on there. gave the appended error.log no hints ?
(In reply to Ralf Habacker from comment #30) > (In reply to Simon McVittie from comment #27) > > Created attachment 119347 [details] [review] [review] [review] > > Don't use _dbus_warn() for intentionally-skipped tests > > > > The tests are run with _dbus_warn() fatal, so if a particular test is > > not applicable on the current platform, we shouldn't call it. > > > > --- > > > > With all these patches applied to dbus-1.10, test-dbus can pass in a > > cross-build "make check" (testing with Debian, Autotools, mingw-w64, Wine > > 1.6.2). > > with cmake test-dbus passed also :-) > 1/1 Test #1: test-dbus ........................ Passed 74.83 sec > > > > > test-bus still fails, with the error you previously described: > > still have the unknown username "root" failure with cmake > > 2: 987: [dbus/dbus-file-win.c(134):_dbus_file_get_contents] file > z:/home/ralf.habacker/src/dbus-1-cmake-cross-x86-build/test/data\valid- > config-files\system.conf hnd 0000008C opened > 2: Unknown username "root" in message bus configuration file > > > > > Z:/srv/tmp/smcv/build/dbus/1.10/mingw/bus/.libs/test-bus.exe: Running SHA1 > > connection test^M > > Expecting NameAcquired, got nothing^M > > > > I don't know what's going on there. > > gave the appended error.log no hints ? From the log it looks that the client expects a signal named NameAcquired to be received, which does not happens in check_hello_message() ... while (!dbus_bus_set_unique_name (connection, name)) _dbus_wait_for_memory (); socd.expected_kind = SERVICE_CREATED; socd.expected_service_name = name; socd.failed = FALSE; socd.skip_connection = connection; /* we haven't done AddMatch so won't get it ourselves */ bus_test_clients_foreach (check_service_owner_changed_foreach, &socd); if (socd.failed) goto out; name_message = message; /* Client should also have gotten ServiceAcquired */ message = pop_message_waiting_for_memory (connection); if (message == NULL) { ->>>> _dbus_warn ("Expecting %s, got nothing\n", "NameAcquired"); goto out; }
(In reply to Ralf Habacker from comment #30) > still have the unknown username "root" failure with cmake I got the CMake part of the patch backwards... will attach a new one. You'll also need to delete any leftover files named system.conf from your build tree.
Created attachment 119352 [details] [review] Don't use _dbus_warn() for intentionally-skipped tests --- add missing #include
Created attachment 119353 [details] [review] Test system bus config files on Unix only --- Under CMake, put system.conf in v-c-f-system/ and the rest in v-c-f/ (matching Autotools), not the other way round
(In reply to Ralf Habacker from comment #30) > gave the appended error.log no hints ? Sorry, no. I've already spent longer on this today than I should, and DBUS_VERBOSE is very good at producing a lot of irrelevant noise, but not particularly good at pointing to the actual problem :-(
Created attachment 119355 [details] [review] Fix missing eol on debug message.
Created attachment 119356 [details] [review] Fix warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement].
Created attachment 119357 [details] [review] Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable].
Comment on attachment 119355 [details] [review] Fix missing eol on debug message. Review of attachment 119355 [details] [review]: ----------------------------------------------------------------- Sure (also, it should be "peer's pid" if we're being pedantic)
Comment on attachment 119356 [details] [review] Fix warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]. Review of attachment 119356 [details] [review]: ----------------------------------------------------------------- Yes please! The spaces around the brackets should ideally be = strlen (address) + 1 ^ ^ ^ if you're touching that line anyway, but no need to fix that at the same time if you don't want to.
Comment on attachment 119357 [details] [review] Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable]. Review of attachment 119357 [details] [review]: ----------------------------------------------------------------- Yes please, but the unused return might also be a bug in its own right. Should there be some sort of if (!GetExitCodeProcess (...)) { /* error handling here */ } or is GetExitCodeProcess not something that can actually fail in practice?
Comment on attachment 119355 [details] [review] Fix missing eol on debug message. committed to dbus-1.10
Comment on attachment 119356 [details] [review] Fix warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]. committed to dbus-1.10 with hints applied
Created attachment 119358 [details] [review] Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable].
Comment on attachment 119352 [details] [review] Don't use _dbus_warn() for intentionally-skipped tests Review of attachment 119352 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 119346 [details] [review] test_default_session_servicedirs: use the intended data directory Review of attachment 119346 [details] [review]: ----------------------------------------------------------------- look good
Comment on attachment 119345 [details] [review] test_default_session_servicedirs: simplify to a single exit code-path committed to dbus-1.10
Comment on attachment 119346 [details] [review] test_default_session_servicedirs: use the intended data directory committed to dbus-1.10
Comment on attachment 119352 [details] [review] Don't use _dbus_warn() for intentionally-skipped tests committed to dbus-1.10
Created attachment 119361 [details] [review] Test system bus config files on Unix only (update 1) - cmake fixes - move valid-config-files/system.d to valid-config-files-system
Created attachment 119362 [details] [review] Test system bus config files on Unix only (update 2) - autotools fixes (location of system.d/test.conf)
Comment on attachment 119362 [details] [review] Test system bus config files on Unix only (update 2) Review of attachment 119362 [details] [review]: ----------------------------------------------------------------- Makes sense. ::: cmake/test/CMakeLists.txt @@ +180,5 @@ > ENDFOREACH(FILE_TYPE) > > MESSAGE(STATUS "Copying generated bus config files to test directory") > +configure_file("${CMAKE_SOURCE_DIR}/../bus/session.conf.in" ${CMAKE_BINARY_DIR}/test/data/valid-config-files/session.conf @ONLY) > +configure_file("${CMAKE_SOURCE_DIR}/../bus/system.conf.in" ${CMAKE_BINARY_DIR}/test/data/valid-config-files-system/system.conf @ONLY) Ah, this is a lot simpler than what I had :-) If the result is equivalent, great, do this.
Comment on attachment 119358 [details] [review] Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable]. Review of attachment 119358 [details] [review]: ----------------------------------------------------------------- OK
Comment on attachment 119358 [details] [review] Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable]. committed to dbus-1.10
Comment on attachment 119362 [details] [review] Test system bus config files on Unix only (update 2) committed to dbus-1.10
(In reply to Simon McVittie from comment #35) > (In reply to Ralf Habacker from comment #30) > > gave the appended error.log no hints ? > > Sorry, no. I've already spent longer on this today than I should, and > DBUS_VERBOSE is very good at producing a lot of irrelevant noise, but not > particularly good at pointing to the actual problem :-( I compared the verbose log file from a test-bus running on unix with the generated from the windows build and got: on unix: 19031: [bus/test.c(251):bus_test_run_bus_loop] ---> Done dispatching on "server side" 19031: [dbus/dbus-transport-socket.c(707):do_writing] wrote 89 bytes of 89 19031: [dbus/dbus-connection.c(660):_dbus_connection_message_sent_unlocked] Message 0x115d1b0 (method_return no path no interface no member 's') removed from outgoing queue 0x 1152510, 0 left to send ... 19031: [dbus/dbus-transport-socket.c(707):do_writing] wrote 169 bytes of 169 19031: [dbus/dbus-connection.c(660):_dbus_connection_message_sent_unlocked] Message 0x115d540 (signal /org/freedesktop/DBus org.freedesktop.DBus NameAcquired 's') removed from outgoing queue 0x1152510, 0 left to send ... 9031: [bus/test.c(208):bus_test_run_clients_loop] ---> Dispatching on "client side" ... 19031: [dbus/dbus-transport-socket.c(887):do_reading] read 258 bytes 19031: [dbus/dbus-marshal-header.c(747):_dbus_header_have_message_untrusted] have 258 bytes, need body 9 + header 80 = 89 ... 19031: [dbus/dbus-marshal-header.c(747):_dbus_header_have_message_untrusted] have 169 bytes, need body 9 + header 160 = 169 windows: 2: 134: [bus/test.c(233):bus_test_run_bus_loop] ---> Dispatching on "server side" ... 2: 134: [dbus/dbus-transport-socket.c(707):do_writing] wrote 89 bytes of 89 2: 134: [dbus/dbus-connection.c(660):_dbus_connection_message_sent_unlocked] Message 00245010 (method_return no path no interface no member 's') removed from outgoing queue 00 246368, 0 left to send ... 2: 134: [dbus/dbus-transport-socket.c(707):do_writing] wrote 169 bytes of 169 2: 134: [dbus/dbus-connection.c(660):_dbus_connection_message_sent_unlocked] Message 0024C980 (signal /org/freedesktop/DBus org.freedesktop.DBus NameAcquired 's') removed from outgoing queue 00246368, 0 left to send ... 2: 134: [bus/test.c(208):bus_test_run_clients_loop] ---> Dispatching on "client side" ... 2: 134: [dbus/dbus-transport-socket.c(887):do_reading] read 89 bytes 2: 134: [dbus/dbus-marshal-header.c(747):_dbus_header_have_message_untrusted] have 89 bytes, need body 9 + header 80 = 89 that are all bytes gotten from the server side. Looks that the data from the NameAcquired signal (169 Bytes) are send from the server side, but not been received by the client - a tcp buffer issue ?
(In reply to Ralf Habacker from comment #56) > 2: 134: [dbus/dbus-transport-socket.c(887):do_reading] read 89 bytes > 2: 134: > [dbus/dbus-marshal-header.c(747):_dbus_header_have_message_untrusted] have > 89 bytes, need body 9 + header 80 = 89 > > that are all bytes gotten from the server side. Looks that the data from the > NameAcquired signal (169 Bytes) are send from the server side, but not been > received by the client - a tcp buffer issue ? here is a related tcpdump: port 55172 is server and port 57349 is client: 07:02:29.974814 IP localhost.55172 > localhost.57349: Flags [.], ack 1, win 342, options [nop,nop,TS val 22231288 ecr 22231288], length 0 07:02:29.976720 IP localhost.55172 > localhost.57349: Flags [P.], seq 1:2, ack 1, win 342, options [nop,nop,TS val 22231290 ecr 22231288], length 1 07:02:29.976730 IP localhost.57349 > localhost.55172: Flags [.], ack 2, win 342, options [nop,nop,TS val 22231290 ecr 22231290], length 0 07:02:29.976759 IP localhost.55172 > localhost.57349: Flags [P.], seq 2:56, ack 1, win 342, options [nop,nop,TS val 22231290 ecr 22231290], length 54 07:02:29.976766 IP localhost.57349 > localhost.55172: Flags [.], ack 56, win 342, options [nop,nop,TS val 22231290 ecr 22231290], length 0 07:02:30.008782 IP localhost.57349 > localhost.55172: Flags [P.], seq 1:47, ack 56, win 342, options [nop,nop,TS val 22231322 ecr 22231290], length 46 07:02:30.008799 IP localhost.55172 > localhost.57349: Flags [.], ack 47, win 342, options [nop,nop,TS val 22231322 ecr 22231322], length 0 07:02:30.009317 IP localhost.55172 > localhost.57349: Flags [P.], seq 56:118, ack 47, win 342, options [nop,nop,TS val 22231323 ecr 22231322], length 62 07:02:30.020530 IP localhost.57349 > localhost.55172: Flags [P.], seq 47:188, ack 118, win 342, options [nop,nop,TS val 22231334 ecr 22231323], length 141 07:02:30.024247 IP localhost.55172 > localhost.57349: Flags [P.], seq 118:271, ack 188, win 350, options [nop,nop,TS val 22231338 ecr 22231334], length 153 07:02:30.024896 IP localhost.57349 > localhost.55172: Flags [P.], seq 188:225, ack 271, win 350, options [nop,nop,TS val 22231338 ecr 22231338], length 37 07:02:30.025574 IP localhost.55172 > localhost.57349: Flags [P.], seq 271:278, ack 225, win 350, options [nop,nop,TS val 22231339 ecr 22231338], length 7 07:02:30.064969 IP localhost.57349 > localhost.55172: Flags [.], ack 278, win 350, options [nop,nop,TS val 22231379 ecr 22231339], length 0 07:02:30.065009 IP localhost.55172 > localhost.57349: Flags [P.], seq 278:406, ack 225, win 350, options [nop,nop,TS val 22231379 ecr 22231379], length 128 07:02:30.065032 IP localhost.57349 > localhost.55172: Flags [.], ack 406, win 359, options [nop,nop,TS val 22231379 ecr 22231379], length 0 07:02:30.071630 IP localhost.57349 > localhost.55172: Flags [P.], seq 225:314, ack 406, win 359, options [nop,nop,TS val 22231385 ecr 22231379], length 89 -> server send package [1] 07:02:30.075732 IP localhost.55172 > localhost.57349: Flags [F.], seq 406, ack 314, win 350, options [nop,nop,TS val 22231389 ecr 22231385], length 0 -> client reads tcp data and closes the connection (sends FIN flag) 07:02:30.075753 IP localhost.57349 > localhost.55172: Flags [P.], seq 314:483, ack 407, win 359, options [nop,nop,TS val 22231389 ecr 22231389], length 169 -> server sends another packet 07:02:30.075796 IP localhost.55172 > localhost.57349: Flags [R], seq 298084616, win 0, length 0 -> client sends RESET flag conclusion: test-bus excepts to gets all messages in *one* tcp package, which is not the case on windows/wine. Therefore the NameAcquired signal get's lost. [1] flags description has been taken https://ask.wireshark.org/questions/31080/tcpdump-output-fpu-and-r-flags
(In reply to Ralf Habacker from comment #57) > conclusion: test-bus excepts to gets all messages in *one* tcp package, > which is not the case on windows/wine. Therefore the NameAcquired signal > get's lost. s/excepts/expects/g, sorry for confusion.
(In reply to Ralf Habacker from comment #58) > (In reply to Ralf Habacker from comment #57) > > conclusion: test-bus excepts to gets all messages in *one* tcp package, > > which is not the case on windows/wine. Therefore the NameAcquired signal > > get's lost. > s/excepts/expects/g, sorry for confusion. Tried to run the test on a native windows 7 and did not get the error. Looks like a bug in wine or a emulate-windows-tcp-on-unix-tcp issue.
(In reply to Ralf Habacker from comment #59) > Tried to run the test on a native windows 7 and did not get the error. Looks > like a bug in wine or a emulate-windows-tcp-on-unix-tcp issue. which is caused by an issue with wine WSASend called in dbus_write_socket_two(). With WINEDEBUG=trace+winsock I get trace:winsock:WS2_sendto socket 0094, wsabuf 0x6cf544, nbufs 2, flags 0, to (nil), tolen 0, ovl (nil), func (nil) trace:winsock:WS2_sendto fd=15, options=0 trace:winsock:WS2_sendto -> 89 bytes trace:winsock:WS2_sendto socket 0094, wsabuf 0x6cf544, nbufs 2, flags 0, to (nil), tolen 0, ovl (nil), func (nil) trace:winsock:WS2_sendto fd=15, options=0 trace:winsock:WS2_sendto -> 169 bytes -> 89+169 = 258 bytes send trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120 trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0 trace:winsock:WS2_recv_base socket 0090, wsabuf 0x6cf808, nbufs 1, flags 0, from (nil), fromlen -1, ovl (nil), func (nil) trace:winsock:WS2_recv_base fd=15, options=0 trace:winsock:WS2_recv_base -> 89 bytes trace:winsock:WS2_recv_base socket 0090, wsabuf 0x6cf808, nbufs 1, flags 0, from (nil), fromlen -1, ovl (nil), func (nil) trace:winsock:WS2_recv_base fd=15, options=0 trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120 --> this is the last winsock calls -> 89 bytes received -> incomplete Replacing WSASend() by two dbus_write_socket calls as already done on the unix variant when WRITEV() is not available, solves the issue. Wine tracing reports then trace:winsock:WS2_sendto -> 80 bytes trace:winsock:WS2_sendto socket 0094, wsabuf 0x6cf4e8, nbufs 1, flags 0, to (nil), tolen 0, ovl (nil), func (nil) trace:winsock:WS2_sendto fd=15, options=0 trace:winsock:WS2_sendto -> 9 bytes trace:winsock:WS2_sendto socket 0094, wsabuf 0x6cf4e8, nbufs 1, flags 0, to (nil), tolen 0, ovl (nil), func (nil) trace:winsock:WS2_sendto fd=15, options=0 trace:winsock:WS2_sendto -> 160 bytes trace:winsock:WS2_sendto socket 0094, wsabuf 0x6cf4e8, nbufs 1, flags 0, to (nil), tolen 0, ovl (nil), func (nil) trace:winsock:WS2_sendto fd=15, options=0 trace:winsock:WS2_sendto -> 9 bytes -> 89+169 = 258 bytes send trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120 trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0 trace:winsock:WS2_recv_base socket 0090, wsabuf 0x6cf808, nbufs 1, flags 0, from (nil), fromlen -1, ovl (nil), func (nil) trace:winsock:WS2_recv_base fd=15, options=0 trace:winsock:WS2_recv_base -> 80 bytes trace:winsock:WS2_recv_base socket 0090, wsabuf 0x6cf808, nbufs 1, flags 0, from (nil), fromlen -1, ovl (nil), func (nil) trace:winsock:WS2_recv_base fd=15, options=0 trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120 trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120 trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120 trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0 trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0 trace:winsock:WS2_recv_base socket 0090, wsabuf 0x6cf808, nbufs 1, flags 0, from (nil), fromlen -1, ovl (nil), func (nil) trace:winsock:WS2_recv_base fd=15, options=0 trace:winsock:WS2_recv_base -> 178 bytes --> 80 +178 = 258 -> all bytes received
(In reply to Ralf Habacker from comment #60) > (In reply to Ralf Habacker from comment #59) > > Tried to run the test on a native windows 7 and did not get the error. Looks > > like a bug in wine or a emulate-windows-tcp-on-unix-tcp issue. > > which is caused by an issue with wine WSASend called in > dbus_write_socket_two(). WSASend() is not problem. Adding a simple delay to dbus_read_socket() in the WSAEWOULDBLOCK case fixes the issue. bytes_read = recv (fd.sock, data, count, 0); if (bytes_read == SOCKET_ERROR) { DBUS_SOCKET_SET_ERRNO(); _dbus_verbose ("recv: failed: %s (%d)\n", _dbus_strerror (errno), errno); + if (errno == WSAEWOULDBLOCK) + Sleep(50); bytes_read = -1; } (In reply to Ralf Habacker from comment #57) > (In reply to Ralf Habacker from comment #56) > > conclusion: test-bus excepts to gets all messages in *one* tcp package, > which is not the case on windows/wine. Therefore the NameAcquired signal get's lost. The main root cause in dbus test case(s) running the server *and* client loop in the same application is the assumption in hello_check_message() (and probably other locations), that all messages send from the server has to be received in *one* client dispatch, which is not the case in all environments. In fact running the test cases on wine brings a design issue in dbus test cases to light.
Created attachment 119419 [details] [review] Fix test cases running client and server dispatch design issue.
Comment on attachment 119419 [details] [review] Fix test cases running client and server dispatch design issue. Review of attachment 119419 [details] [review]: ----------------------------------------------------------------- Thanks for analyzing this. The fix makes sense in general. ::: bus/dispatch.c @@ +676,5 @@ > if (message == NULL) > { > + dbus_connection_ref (connection); > + block_connection_until_message_from_bus (d->context, connection, "NameOwnerChanged"); > + dbus_connection_unref (connection); Why this ref/unref? Aren't we holding a ref at this point anyway? If they are needed, shouldn't the unref be *after* we call pop_message_waiting_for_memory (connection)?
(In reply to Simon McVittie from comment #63) > Comment on attachment 119419 [details] [review] [review] > Fix test cases running client and server dispatch design issue. > > Review of attachment 119419 [details] [review] [review]: > ----------------------------------------------------------------- > > Thanks for analyzing this. The fix makes sense in general. > > ::: bus/dispatch.c > @@ +676,5 @@ > > if (message == NULL) > > { > > + dbus_connection_ref (connection); > > + block_connection_until_message_from_bus (d->context, connection, "NameOwnerChanged"); > > + dbus_connection_unref (connection); > > Why this ref/unref? Aren't we holding a ref at this point anyway? no, it has been unref'd at http://cgit.freedesktop.org/dbus/dbus/tree/bus/dispatch.c?h=dbus-1.10#n944 . A bug ? > If they are needed, shouldn't the unref be *after* we call > pop_message_waiting_for_memory (connection)? copied from http://cgit.freedesktop.org/dbus/dbus/tree/bus/dispatch.c?h=dbus-1.10#n933
Created attachment 119437 [details] [review] Fix bug unrefing connection too early in check_hello_message().
Created attachment 119438 [details] [review] Fix test cases running client and server dispatch design issue. requires attachment 119437 [details] [review]
Created attachment 119439 [details] [review] Fix test cases running client and server dispatch design issue. removed obsolate BusContext struct member
Comment on attachment 119439 [details] [review] Fix test cases running client and server dispatch design issue. revert this patch, context struct member is required
Comment on attachment 119437 [details] [review] Fix bug unrefing connection too early in check_hello_message(). Review of attachment 119437 [details] [review]: ----------------------------------------------------------------- Go ahead. OK for 1.10, too.
Comment on attachment 119438 [details] [review] Fix test cases running client and server dispatch design issue. Review of attachment 119438 [details] [review]: ----------------------------------------------------------------- Looks good now. Also fine for 1.10 I think.
Comment on attachment 119437 [details] [review] Fix bug unrefing connection too early in check_hello_message(). committed to dbus-1.10 and merged into master
Comment on attachment 119438 [details] [review] Fix test cases running client and server dispatch design issue. committed to dbus-1.10 and merged into master
(In reply to Ralf Habacker from comment #72) > Comment on attachment 119438 [details] [review] [review] > Fix test cases running client and server dispatch design issue. > > committed to dbus-1.10 and merged into master Remaining issue is a memory leak: S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for memleaks S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: Running service files reloading test S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for memleaks 1 dbus_malloc blocks were not freed Unit test failed: memleaks
(In reply to Ralf Habacker from comment #73) > Remaining issue is a memory leak: > > S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for > memleaks > S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: Running service > files reloading test > S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for > memleaks > 1 dbus_malloc blocks were not freed > Unit test failed: memleaks Yeah. This is weird: I wasn't seeing memory leaks reported when I ran this stuff last week, but I ran the tests again yesterday and the Windows cross-builds were leaking. I'm not sure why; nothing we've done recently jumped out at me as a likely source of Windows-specific leaks. The Unix builds aren't leaking, so my usual leak-debugging techniques (valgrind etc.) are useless here. Do you have better Windows debug tools available?
(In reply to Simon McVittie from comment #74) > (In reply to Ralf Habacker from comment #73) > > Remaining issue is a memory leak: > > > > S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for > > memleaks > > S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: Running service > > files reloading test > > S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for > > memleaks > > 1 dbus_malloc blocks were not freed > > Unit test failed: memleaks > > Yeah. This is weird: I wasn't seeing memory leaks reported when I ran this > stuff last week, but I ran the tests again yesterday and the Windows > cross-builds were leaking. I'm not sure why; nothing we've done recently > jumped out at me as a likely source of Windows-specific leaks. <> > The Unix builds aren't leaking, so my usual leak-debugging techniques > (valgrind etc.) are useless here. Do you have better Windows debug tools > available? I added some test code using a map to track malloc/free's and got: S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for memleaks S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: Running service files reloading test 00246098 malloc (File not found.) There is an error case where a dbus string is not free'd.
Created attachment 119478 [details] [review] Fix memory leak in _dbus_win_set_error_from_win_error() with zero error parameter.
Created attachment 119479 [details] [review] Fix memory leaks in bus_activation_service_reload_test() in case of errors.
Just for record: To be able to see debug symbols in winedbg I need to cross compile dbus with '-gdwarf-2 -gstrict-dwarf'.
(In reply to Ralf Habacker from comment #76) > Created attachment 119478 [details] [review] [review] > Fix memory leak in _dbus_win_set_error_from_win_error() with zero error > parameter. This patch is the required one, the other are fixes I found too while analyzing.
Created attachment 119488 [details] [review] Not not abort test-bus on printing windows todo messages.
Created attachment 119490 [details] [review] Fix test-bus segfault_service_no_auto_start test on windows.
Created attachment 119491 [details] [review] Add tests for GetConnectionWindowsUser and GetConnectionProcessID to test-bus for windows.
Comment on attachment 119488 [details] [review] Not not abort test-bus on printing windows todo messages. superseeded by attachment 119491 [details] [review]
Comment on attachment 119478 [details] [review] Fix memory leak in _dbus_win_set_error_from_win_error() with zero error parameter. Review of attachment 119478 [details] [review]: ----------------------------------------------------------------- Well caught, but I don't think this is actually the right fix: I think the current implementation is just wrong, and would leak memory whether error is NULL or not. > if (msg) > { > char *msg_copy; > > msg_copy = dbus_malloc (strlen (msg)); This doesn't allow space for the '\0' at the end, so we're probably overrunning msg_copy by 1 byte. > strcpy (msg_copy, msg); strcpy() is usually wrong. We prefer DBusString, or strncpy() if DBusString really can't be used for some reason. > LocalFree (msg); > > dbus_set_error (error, "win32.error", "%s", msg_copy); msg_copy is never freed here, as far as I can see, so it's leaked. Also, I don't think we even needed to copy it, because dbus_set_error() copies its arguments! I think the correct fix would be something like this: FormatMessageA (..., &msg, ...); if (msg) { dbus_set_error (error, "win32.error", "%s", msg); } else ... what we have now is fine ... ("win32.error" should really also be replaced by one of the documented error names like DBUS_ERROR_FILE_EXISTS, based on a switch() over the error code, but that's a separate bug.)
Comment on attachment 119479 [details] [review] Fix memory leaks in bus_activation_service_reload_test() in case of errors. Review of attachment 119479 [details] [review]: ----------------------------------------------------------------- Looks good, although I slightly prefer the pattern of dbus_bool_t ret = FALSE; if (something failed) goto out; if (something else failed) goto out; ret = TRUE; out: free (stuff); free (other stuff); return ret; (effectively a C implementation of the C++ try/finally construct)
Comment on attachment 119490 [details] [review] Fix test-bus segfault_service_no_auto_start test on windows. Review of attachment 119490 [details] [review]: ----------------------------------------------------------------- ::: bus/dispatch.c @@ +3022,5 @@ > +#ifdef DBUS_WIN > + else if (dbus_message_is_error (message, > + DBUS_ERROR_SPAWN_CHILD_EXITED)) > + { > + ; /* good, this is expected also */ Am I right in thinking that this is expected because on Unix we can reliably tell the difference between a child that was killed by a signal and a child that gracefully exited nonzero, but on Windows both of those are just treated as a nonzero exit? If that's what is going on here, then the change is fine, but I'd prefer to have a comment explaining why the expected result is different on Unix vs Windows. I know you're just copying what the current code does, but the semicolon is unnecessary here: it's fine to have a block containing only a comment, like else if (...) { /* Windows exit codes don't distinguish between segfault and * graceful unsuccessful exit in the same way Unix does */ } and some compilers will warn about the empty statement.
Comment on attachment 119491 [details] [review] Add tests for GetConnectionWindowsUser and GetConnectionProcessID to test-bus for windows. Review of attachment 119491 [details] [review]: ----------------------------------------------------------------- The commit message seems rather backwards: the primary thing you're doing here is adding new functionality (the new methods); secondarily, you're testing it. I don't want these two new methods, because GetConnectionCredentials() covers both of them, and only needs one round-trip. ::: bus/driver.c @@ +2227,5 @@ > DBUS_TYPE_STRING_AS_STRING, > DBUS_TYPE_ARRAY_AS_STRING DBUS_TYPE_STRING_AS_STRING, > bus_driver_handle_list_queued_owners }, > +#ifdef DBUS_WIN > + { "GetConnectionWindowsUser", Please don't add new methods to get individual credentials. They lead to unnecessary round-trips in the common case that a caller wants to know multiple credentials (user ID, process ID and LSM context on Unix; SID and process ID on Windows). New code should use GetConnectionCredentials() on all platforms. We're only keeping the Unix-specific ones for backwards compatibility with things that already rely on them. If new methods are added to the o.fd.DBus interface, they should not be platform-specific: the contents of an interface should not change on different platforms. If a method cannot work on a particular platform, it should exist but return an error, just like GetConnectionUnixUser() returns an error on Windows. @@ +2231,5 @@ > + { "GetConnectionWindowsUser", > + DBUS_TYPE_STRING_AS_STRING, > + DBUS_TYPE_UINT32_AS_STRING, > + bus_driver_handle_get_connection_windows_user }, > + { "GetConnectionProcessID", Definitely don't add this, because GetConnectionUnixProcessID() (despite its name, which we have to keep for historical reasons) would be fine to make cross-platform.
Created attachment 119509 [details] [review] Fix memory leak in _dbus_win_set_error_from_win_error() (update 1)
Created attachment 119510 [details] [review] Fix memory leaks in bus_activation_service_reload_test() in case of errors.
(In reply to Simon McVittie from comment #86) Y > Am I right in thinking that this is expected because on Unix we can reliably > tell the difference between a child that was killed by a signal and a child > that gracefully exited nonzero, but on Windows both of those are just > treated as a nonzero exit? > yes from https://msdn.microsoft.com/de-de/library/windows/desktop/ms683189%28v=vs.85%29.aspx "... If the process has terminated and the function succeeds, the status returned is one of the following values: The exit value specified in the ExitProcess or TerminateProcess function. The return value from the main or WinMain function of the process. The exception value for an unhandled exception that caused the process to terminate." > If that's what is going on here, then the change is fine, but I'd prefer to > have a comment explaining why the expected result is different on Unix vs > Windows. ... because on unix DBUS_ERROR_SPAWN_CHILD_SIGNALED indicates a signaled child, which could also be used on windows if we find a way to distinguish normal exit codes from signal codes at http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-spawn-win.c?h=dbus-1.10#n343. One indication are the high negative values for example 0xc00000005 -> segfault > > I know you're just copying what the current code does, but the semicolon is > unnecessary here: will fix this
(In reply to Ralf Habacker from comment #90) > > ... because on unix DBUS_ERROR_SPAWN_CHILD_SIGNALED indicates a signaled > child, which could also be used on windows if we find a way to distinguish > normal exit codes from signal codes at > http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-spawn-win.c?h=dbus-1. > 10#n343. One indication are the high negative values for example 0xc00000005 > -> segfault cygwin for example detects an unhandled exception by comparing the exit code with >= 0xc00000000 https://cygwin.com/ml/cygwin-patches/2008-q1/msg00063.html.
Created attachment 119522 [details] [review] Fix test-bus segfault_service_no_auto_start test on windows (update 1) Note: Using DBUS_ERROR_SPAWN_CHILD_EXITED looks more natural on windows as adding an extra emulation layer.
(In reply to Simon McVittie from comment #87) > > I don't want these two new methods, because GetConnectionCredentials() > covers both of them, and only needs one round-trip. Then those tests should be skipped on windows ?
Created attachment 119523 [details] [review] Skip deprecated test-bus tests for GetConnectionUnixUser and GetConnectionUnixProcessID on Windows.
Created attachment 119529 [details] [review] Skip deprecated test-bus tests for GetConnectionUnixUser and GetConnectionUnixProcessID on Windows. fixed wrong base
Created attachment 119530 [details] [review] Do not use dbus_warn for skipping unit tests on windows to avoid fatal errors.
Comment on attachment 119522 [details] [review] Fix test-bus segfault_service_no_auto_start test on windows (update 1) Review of attachment 119522 [details] [review]: ----------------------------------------------------------------- Looks good, thanks
Comment on attachment 119522 [details] [review] Fix test-bus segfault_service_no_auto_start test on windows (update 1) Review of attachment 119522 [details] [review]: ----------------------------------------------------------------- found little spelling issue s/and by/and not by/. Will commit with this fix
Comment on attachment 119522 [details] [review] Fix test-bus segfault_service_no_auto_start test on windows (update 1) committed to dbus-1.10
Comment on attachment 119530 [details] [review] Do not use dbus_warn for skipping unit tests on windows to avoid fatal errors. Review of attachment 119530 [details] [review]: ----------------------------------------------------------------- OK. It would also be OK to skip testing the launch helper without any comment at all: it makes no sense on Windows (the system bus is unsupported there, and I don't think Windows has any concept of setuid binaries, so it can't ever actually work).
Comment on attachment 119529 [details] [review] Skip deprecated test-bus tests for GetConnectionUnixUser and GetConnectionUnixProcessID on Windows. Review of attachment 119529 [details] [review]: ----------------------------------------------------------------- The functional change is fine, but these tests are not actually deprecated as such. The methods that they're testing are weakly deprecated in favour of GetConnectionCredentials(), but must still be supported on Unix. On Windows, they're unimportant, and I don't mind continuing to not test them on Windows: presumably they did not traditionally work anyway, and they're mainly useful for the system bus, which really only makes sense on Unix. GetConnectionUnixUser() is meaningless on Windows and should just raise an error. Ideally, its test would verify that it raises an error #ifdef DBUS_WIN, or succeeds otherwise. "Out of memory" is a valid result either way, because of how these tests are structured. I think GetConnectionUnixProcessID() might actually be able to succeed on Windows now (despite its name!) since you added that functionality a few months ago? But if it fails, that's OK too. Again, "out of memory" is also a valid result on either platform.
Comment on attachment 119509 [details] [review] Fix memory leak in _dbus_win_set_error_from_win_error() (update 1) Review of attachment 119509 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +2377,4 @@ > # define BACKTRACES > #endif > > +#undef BACKTRACES Why? If the backtrace code is broken or otherwise not useful (I've never had it work on Wine, but perhaps I'm using wrong toolchain settings), we should just delete it. If it's useful, we can keep it. @@ +3604,5 @@ > { > char *msg; > > + if (!error) > + return; OK: minor optimization (not doing unnecessary work), harmless. Ideally this would be a separate commit though. @@ +3615,4 @@ > (LPSTR) &msg, 0, NULL); > if (msg) > { > + dbus_set_error (error, "win32.error", "%s", msg); OK: this is the actual bug fix.
Comment on attachment 119510 [details] [review] Fix memory leaks in bus_activation_service_reload_test() in case of errors. Review of attachment 119510 [details] [review]: ----------------------------------------------------------------- Yes please!
Created attachment 119562 [details] [review] Fix memory leak in _dbus_win_set_error_from_win_error() (update 2)
Comment on attachment 119510 [details] [review] Fix memory leaks in bus_activation_service_reload_test() in case of errors. committed to dbus-1.10
Comment on attachment 119562 [details] [review] Fix memory leak in _dbus_win_set_error_from_win_error() (update 2) Review of attachment 119562 [details] [review]: ----------------------------------------------------------------- Perfect
Comment on attachment 119562 [details] [review] Fix memory leak in _dbus_win_set_error_from_win_error() (update 2) committed to dbus-1.10
Created attachment 119575 [details] [review] Complete support for dbus-daemon driver methods GetConnectionUnixUser and GetConnectionUnixProcessID on windows.
(In reply to Simon McVittie from comment #101) > GetConnectionUnixUser() is meaningless on Windows and should just raise an > error. Ideally, its test would verify that it raises an error #ifdef > DBUS_WIN, or succeeds otherwise. "Out of memory" is a valid result either > way, because of how these tests are structured. I choosed the already available error code DBUS_ERROR_NOT_SUPPORTED. > I think GetConnectionUnixProcessID() might actually been able to succeed on > Windows now (despite its name!) since you added that functionality a few > months ago? Recent patch enable it for windows, but after implementing I'm coming to the conclusion, that it should better fail and return a hint to use GetConnectionCredentials instead. The hint would also be useful for GetConnectionUnixUser.
(In reply to Simon McVittie from comment #102) > > +#undef BACKTRACES > removed from patch > > If the backtrace code is broken or otherwise not useful (I've never had it > work on Wine, but perhaps I'm using wrong toolchain settings), we should > just delete it. If it's useful, we can keep it. Need to take a deeper look.
Created attachment 119576 [details] [review] Skip launch helper activation tests on windows silently.
Comment on attachment 119576 [details] [review] Skip launch helper activation tests on windows silently. Review of attachment 119576 [details] [review]: ----------------------------------------------------------------- OK
Comment on attachment 119575 [details] [review] Complete support for dbus-daemon driver methods GetConnectionUnixUser and GetConnectionUnixProcessID on windows. Review of attachment 119575 [details] [review]: ----------------------------------------------------------------- ::: bus/dispatch.c @@ -1421,5 @@ > - { > - /* We are expecting this error, since we know in the test suite we aren't > - * talking to a client running on UNIX > - */ > - _dbus_verbose ("Windows correctly does not support GetConnectionUnixProcessID\n"); If we now have a fully working GetConnectionUnixProcessID() on Windows (i.e. it returns the pid of the other end of the connection), then this part is fine. @@ -1446,5 @@ > else > { > -#ifdef DBUS_WIN > - warn_unexpected (connection, message, "GetConnectionUnixProcessID to fail on Windows"); > - goto out; This too. @@ -4838,5 @@ > #endif > > -#ifdef DBUS_WIN > - _dbus_verbose("Skipping deprecated test for GetConnectionUnixUser\n"); > - _dbus_verbose("Skipping deprecated test for GetConnectionUnixProcessID\n"); This part is fine, assuming you can make those two tests pass (either by changing the implementation or by changing the expectation) ::: bus/driver.c @@ +1463,5 @@ > +#ifdef DBUS_WIN > + dbus_set_error (error, > + DBUS_ERROR_NOT_SUPPORTED, > + "Not supported on windows platform"); > + goto failed; You shouldn't need an explicit #ifdef DBUS_WIN here, because dbus_connection_get_unix_user() already returns FALSE whenever we don't know the uid of the other end of the connection. On Windows, the other end of the connection doesn't *have* a uid (there is no such thing). It looks as though we currently raise DBUS_ERROR_FAILED for that, the same as we would on Unix for a connection that has used ANONYMOUS authentication. I think that error is fine: there's no need for a special case for Windows here.
Comment on attachment 119576 [details] [review] Skip launch helper activation tests on windows silently. committed to dbus-1.10
Comment on attachment 119529 [details] [review] Skip deprecated test-bus tests for GetConnectionUnixUser and GetConnectionUnixProcessID on Windows. required for attachment 119575 [details] [review]
(In reply to Simon McVittie from comment #113) > ::: bus/driver.c > @@ +1463,5 @@ > > +#ifdef DBUS_WIN > > + dbus_set_error (error, > > + DBUS_ERROR_NOT_SUPPORTED, > > + "Not supported on windows platform"); > > + goto failed; > > You shouldn't need an explicit #ifdef DBUS_WIN here, because > dbus_connection_get_unix_user() already returns FALSE whenever we don't know > the uid of the other end of the connection. > > On Windows, the other end of the connection doesn't *have* a uid (there is > no such thing). It looks as though we currently raise DBUS_ERROR_FAILED for > that, the same as we would on Unix for a connection that has used ANONYMOUS > authentication. I think that error is fine: there's no need for a special case for Windows here. recent implementation of check_get_connection_unix_user() do not check the DBUS_ERROR_FAILED error: 8: [bus/dispatch.c(640):verbose_message_received] Received message interface "(unset)" member "(unset)" error name "org.freedesktop.DBus.Error.Failed" on 0024F4A8 check_get_connection_unix_user:1290 received message interface "(unset)" member "(unset)" error name "org.freedesktop.DBus.Error.Failed" on 0024F4A8, expecting not this error It is required to add a dedicated catch. Also the message returned from the driver: "Could not determine UID for '%s'", service); is very unspecific and forces the user to take additional time to find out the real reason. Instead returning "Not supported, use GetConnectionCredentials instead"); would give a helpful explanation of the problem.
(In reply to Ralf Habacker from comment #116) > recent implementation of check_get_connection_unix_user() do not check the > DBUS_ERROR_FAILED error: > > 8: [bus/dispatch.c(640):verbose_message_received] Received message interface > "(unset)" member "(unset)" error name "org.freedesktop.DBus.Error.Failed" on > 0024F4A8 > check_get_connection_unix_user:1290 received message interface "(unset)" > member "(unset)" error name "org.freedesktop.DBus.Error.Failed" on 0024F4A8, > expecting not this error > > It is required to add a dedicated catch. OK, then something like #ifdef DBUS_WIN else if (it is FAILED) { /* this is OK, Unix uids aren't meaningful on Windows */ } #endif in the test-case would be appropriate. > Also the message returned from the driver: > > "Could not determine UID for '%s'", service); > > is very unspecific and forces the user to take additional time to find out > the real reason. What would it mean for a peer to have a Unix uid on a Windows system? What Unix uid could it possibly have? What would a caller do with that uid? They can't compare it with the result of geteuid(), because Windows doesn't have that function. They can't usefully compare it with (struct stat)->st_uid, because that struct member is always 0 on Windows. And so on. > Instead returning > > "Not supported, use GetConnectionCredentials instead"); > > would give a helpful explanation of the problem. Not particularly helpful. If GetConnectionUnixUser() fails, then UnixUserID would not be in the result of GetConnectionCredentials() either: they give the same information, in a different format. If you are working with identity information on Windows, then you need to know that the operating system represents that as a string SID instead of a numeric uid, and use APIs that give you that information (like GetConnectionCredentials)... but presumably you're doing something with that information, like comparing it with your own SID, in which case you already need to know that SIDs are strings. GetConnectionCredentials and its semi-deprecated ancestors are mainly useful on the system bus, which Windows doesn't have. The session bus is not designed to be a security boundary, so session services can assume that every peer has the same user ID (uid or SID) that the service does, because the session bus does not have <allow user="*"/>; that means connections that do not have the same uid or SID are rejected (auth_via_default_rules() in dbus/dbus-transport.c calls _dbus_credentials_same_user() which knows about both uids and SIDs).
Created attachment 119649 [details] [review] Fix test-bus test for GetConnectionUnixUser driver method on windows.
Created attachment 119650 [details] [review] Fix test-bus test for GetConnectionUnixUser driver method on windows (update 1)
Created attachment 119651 [details] [review] Do not fail with fatal message skipping GetConnectionUnixProcessID test-bus test on windows.
Comment on attachment 119650 [details] [review] Fix test-bus test for GetConnectionUnixUser driver method on windows (update 1) Review of attachment 119650 [details] [review]: ----------------------------------------------------------------- Nice. OK for 1.10 too
Comment on attachment 119651 [details] [review] Do not fail with fatal message skipping GetConnectionUnixProcessID test-bus test on windows. Review of attachment 119651 [details] [review]: ----------------------------------------------------------------- Fine ::: bus/dispatch.c @@ +4847,3 @@ > _dbus_assert_not_reached ("GetConnectionUnixUser message failed"); > > #ifdef DBUS_WIN_FIXME Out of interest, where do we #define this, and can we make it just be DBUS_WIN?
Comment on attachment 119650 [details] [review] Fix test-bus test for GetConnectionUnixUser driver method on windows (update 1) committed to dbus-1.10
Comment on attachment 119651 [details] [review] Do not fail with fatal message skipping GetConnectionUnixProcessID test-bus test on windows. committed to dbus-1.10
(In reply to Simon McVittie from comment #122) > #ifdef DBUS_WIN_FIXME > > Out of interest, where do we #define this, cmake/config.h.cmake > and can we make it just be DBUS_WIN? The recent situation looks somehow unfinished: 1. By implementing GetConnectionCredentials on windows deprecated method GetConnectionUnixProcessID has also been enabled, is available on windows and may be used by applications. 2. By reverting this implicit enable and let GetConnectionUnixProcessID now fail on windows may break applications, so it has to stay :-( 3. Persistently skipping the testcase for the working GetConnectionUnixProcessID on windows by renaming the define to DBUS_WIN may hide regressions introduced in the future. BTW: DBUS_WIN_FIXME is also defined in _dbus_decrement_fail_alloc_counter.
Created attachment 119655 [details] [review] Fix recursive loop in _dbus_abort() in case DBUS_FATAL_WARNINGS=1 on windows. running bus-test with environment variable DBUS_FATAL_WARNINGS=1 and calls to dbus_warn() (for example without attachment 119651 [details] [review]) prints out Backtrace: Backtrace: Backtrace: Backtrace: Backtrace: Backtrace: ...
(In reply to Ralf Habacker from comment #110) > > If the backtrace code is broken or otherwise not useful (I've never had it > > work on Wine, but perhaps I'm using wrong toolchain settings), we should > > just delete it. If it's useful, we can keep it. > Need to take a deeper look. backtrace support needs attachment 119655 [details] [review]
Comment on attachment 119655 [details] [review] Fix recursive loop in _dbus_abort() in case DBUS_FATAL_WARNINGS=1 on windows. Review of attachment 119655 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps.c @@ +79,5 @@ > const char *s; > + static dbus_bool_t active = FALSE; > + > + if (active) > + return; This makes me uncomfortable, because _dbus_abort() should never return. @@ +86,1 @@ > _dbus_print_backtrace (); Instead of doing this, I'd prefer to make the Windows implementation of _dbus_print_backtrace() use fprintf (stderr, ...) instead of _dbus_warn (...). The author of the Unix implementation was aware of this issue: it has this comment: /* don't use dbus_warn since it can _dbus_abort() */ fprintf (stderr, " %s\n", syms[i]); but unfortunately the same is not true of the Windows implementation. As far as I can see, the necessary change to dbus-sysdeps-win.c would be to change #define DPRINTF _dbus_warn to something like #define DPRINTF(fmt, ...) fprintf (stderr, fmt, ##__VA_ARGS__) or alternatively, replace every call to DPRINTF with the corresponding fprintf to stderr. DPRINTF is only used in the backtracing code, as far as I can tell.
(In reply to Ralf Habacker from comment #125) > 1. By implementing GetConnectionCredentials on windows deprecated method > GetConnectionUnixProcessID has also been enabled, is available on windows > and may be used by applications. That's fine. The method is deprecated, but we have to continue to support it (at least on Unix) anyway, because applications use it; and 99% of the implementation is shared with GetConnectionCredentials(). If dbus-daemon on Windows returns a process ID in GetConnectionCredentials(), then there is no real harm in having GetConnectionUnixProcessID() also work (despite its name) - again, 99% of the implementation is needed for GetConnectionCredentials() anyway. The method name was presumably chosen because the author of the relevant bit of the specification wasn't sure whether there was an equivalent of pid_t on non-Unix, but when we discussed it during implementation of GetConnectionCredentials(), we came to the conclusion that Unix pid_t and Windows process IDs are essentially equivalent - they're a positive integer that identifies a local process - so there's no real point in distinguishing.
Created attachment 119689 [details] [review] Fix recursive loop in backtrace generator on windows.
Comment on attachment 119689 [details] [review] Fix recursive loop in backtrace generator on windows. Review of attachment 119689 [details] [review]: ----------------------------------------------------------------- Looks good. I think this is sufficiently narrow for 1.10.
Comment on attachment 119689 [details] [review] Fix recursive loop in backtrace generator on windows. committed to dbus-1.10
Created attachment 119710 [details] [review] Print stack index in backtrace generator on Windows.
Created attachment 119711 [details] [review] Add file and line support to backtrace generator on windows.
Created attachment 119712 [details] [review] Add backtrace test app on windows.
Created attachment 119713 [details] [review] Enable debug symbols on wine.
Comment on attachment 119710 [details] [review] Print stack index in backtrace generator on Windows. Review of attachment 119710 [details] [review]: ----------------------------------------------------------------- OK, but only for master.
Comment on attachment 119711 [details] [review] Add file and line support to backtrace generator on windows. Review of attachment 119711 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +2550,5 @@ > + HANDLE hProcess, > + DWORD dwAddr, > + PDWORD pdwDisplacement, > + PIMAGEHLP_LINE Line > +))GetProcAddress (hmodDbgHelp, FUNC(SymGetLineFromAddr)); This indentation is really quite weird. It's functionally OK, but I'd prefer something like this: /* globally */ typedef BOOL (WINAPI *SymGetLineFromAddrImpl) (HANDLE hProcess, DWORD dwAddr, PDWORD pdwDisplacement, PIMAGEHLP_LINE Line); static SymGetLineFromAddrImpl pSymGetLineFromAddr; /* in the function */ pSymGetLineFromAddr = (SymGetLineFromAddrImpl) GetProcAddress (hmodDbgHelp, FUNC (SymGetLineFromAddr)); @@ +2638,5 @@ > > pSymbol->SizeOfStruct = sizeof(IMAGEHLP_SYMBOL); > pSymbol->MaxNameLength = sizeof(buffer) - sizeof(IMAGEHLP_SYMBOL) + 1; > > + if (pSymGetSymFromAddr(GetCurrentProcess(), sf.AddrPC.Offset, Whitespace nitpicking: pSymGetEtcEtc (GetCurrentProcess (), sf.AddrPC.Offset, ^ ^ with the spaces before the opening parentheses @@ +2650,2 @@ > else > + DPRINTF ("%3d 0x%lx", i++, sf.AddrPC.Offset); Is Offset guaranteed to be the same size as long, even on 64-bit? @@ +2650,5 @@ > else > + DPRINTF ("%3d 0x%lx", i++, sf.AddrPC.Offset); > + > + line.SizeOfStruct = sizeof(IMAGEHLP_LINE); > + if (pSymGetLineFromAddr(GetCurrentProcess(), sf.AddrPC.Offset, &dwDisplacement, &line) == TRUE) We normally avoid "== TRUE". If the function returns a boolean result, it's unnecessary; if the function returns a multi-valued integer so the distinction between "nonzero" and "exactly 1" matters, use "== 1" or "== SOME_ENUM_MEMBER". @@ +2652,5 @@ > + > + line.SizeOfStruct = sizeof(IMAGEHLP_LINE); > + if (pSymGetLineFromAddr(GetCurrentProcess(), sf.AddrPC.Offset, &dwDisplacement, &line) == TRUE) > + { > + DPRINTF (" [%s:%ld]", line.FileName, line.LineNumber); Is LineNumber guaranteed to be the same size as long, even on 64-bit?
Comment on attachment 119712 [details] [review] Add backtrace test app on windows. Review of attachment 119712 [details] [review]: ----------------------------------------------------------------- ::: cmake/test/CMakeLists.txt @@ +74,4 @@ > 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-backtrace ${CMAKE_SOURCE_DIR}/../test/manual-backtrace.c ${DBUS_INTERNAL_LIBRARIES} dbus-1) I don't think this needs ${DBUS_INTERNAL_LIBRARIES} - if you need to put DBUS_PRIVATE_EXPORT on the symbols, then they must be part of libdbus-1. Is there any reason why this can't be built on Unix too, like manual-tcp? Please build it in Autotools too (search for for manual-tcp in test/Makefile.am and do the same things).
Comment on attachment 119713 [details] [review] Enable debug symbols on wine. Review of attachment 119713 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ +225,5 @@ > SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /w14018") > SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /w14018") > else() > + SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wsign-compare -gdwarf-2") > + SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wsign-compare -gdwarf-2") Is it sufficient to use plain "-g", which is the general-purpose flag for adding debug symbols? Shouldn't this be either part of configuring CMake for the DEBUG build-type (CMAKE_C_FLAGS_DEBUG, which I think uses -g anyway), or conditional on NOT DBUS_DISABLE_ASSERT?
(In reply to Simon McVittie from comment #139) > Is there any reason why this can't be built on Unix too, like manual-tcp? No, did not saw that there is also a backtrace generator on linux. > Please build it in Autotools too (search for for manual-tcp in > test/Makefile.am and do the same things). sure
Created attachment 119715 [details] [review] Add backtrace test app on windows. - autotools support - fixes
Created attachment 119716 [details] [review] Add backtrace test app. - commit message fix
Created attachment 119725 [details] [review] Refactor backtrace generator followup of attachment 119711 [details] [review]
Comment on attachment 119716 [details] [review] Add backtrace test app. Review of attachment 119716 [details] [review]: ----------------------------------------------------------------- Looks good for master.
Comment on attachment 119710 [details] [review] Print stack index in backtrace generator on Windows. committed to master
Comment on attachment 119716 [details] [review] Add backtrace test app. committed to master
Comment on attachment 119725 [details] [review] Refactor backtrace generator Review of attachment 119725 [details] [review]: ----------------------------------------------------------------- I need to resent an updated patch. I tried to git am'd it on another machine, which fails.
Comment on attachment 119725 [details] [review] Refactor backtrace generator Review of attachment 119725 [details] [review]: ----------------------------------------------------------------- ::: cmake/dbus/CMakeLists.txt @@ +275,4 @@ > if(WINCE) > target_link_libraries(dbus-1 ws2) > else(WINCE) > + target_link_libraries(dbus-1 ws2_32 advapi32 netapi32 iphlpapi dbghelp) 100% less dlopen()-equivalent. I like it! ::: dbus/dbus-sysdeps-win.c @@ +2474,5 @@ > + while (StackWalk (dwImageType, GetCurrentProcess (), > + hThread, &sf, &context, NULL, SymFunctionTableAccess, > + SymGetModuleBase, NULL)) > + { > + BYTE buffer[256]; I'd prefer this to be of length sizeof (IMAGEHLP_SYMBOL) + some arbitrary constant, to guarantee that it is strictly larger than IMAGEHLP_SYMBOL. @@ +2488,5 @@ > + IMAGEHLP_MODULE moduleInfo; > + DWORD dwDisplacement; > + > +#ifdef _IMAGEHLP64 > + /* wine hack */ Please include a link to a relevant bug report or something, to explain what it is that you're hacking around? Or if there is none, either briefly describe it in the comment itself, or explain it on a comment on this bug and link to that. I infer from the code that it's something like "on Wine64, we would sometimes get an infinite number of stack entries pointing to the same stack frame"? @@ +2493,5 @@ > + if (old_address == sf.AddrPC.Offset) > + break; > +#endif > + pSymbol->SizeOfStruct = sizeof (IMAGEHLP_SYMBOL); > + pSymbol->MaxNameLength = sizeof (buffer) - sizeof (IMAGEHLP_SYMBOL) + 1; Should this be sizeof (buffer) - sizeof (IMAGEHLP_SYMBOL) - 1, or equivalently sizeof (buffer) - (sizeof (IMAGEHLP_SYMBOL) + 1), to allow space for an IMAGEHLP_SYMBOL structure followed by MaxNameLength bytes followed by '\0'? @@ +2508,2 @@ > > + line.SizeOfStruct = sizeof (IMAGEHLP_LINE); I'd slightly prefer sizeof (line) so that it very obviously matches, but this isn't important. @@ +2514,5 @@ > > + moduleInfo.SizeOfStruct = sizeof (moduleInfo); > + if (SymGetModuleInfo (GetCurrentProcess (), sf.AddrPC.Offset, &moduleInfo)) > + { > + DPRINTF (" in %s\n", moduleInfo.ModuleName); This is actually a mistake in the earlier patches on this bug, which I'm only noticing now: if SymGetModuleInfo() fails, you won't print a newline. I'd prefer: if (SymGetModuleInfo (...) { DPRINTF ("in %s", ...); } old_address = sf.AddrPC.Offset; DPRINTF ("\n"); so that we are obviously printing one newline per logical line. @@ +2526,2 @@ > { > + dump_backtrace_for_thread ((HANDLE)lpParameter); Not very important, but libdbus style for casts is (HANDLE) lpParameter, with a space.
Created attachment 119737 [details] [review] Add file and line support to backtrace generator on windows. (update1) Fix eol output in case module could not be retrieved.
Created attachment 119738 [details] [review] Refactor windows backtrace generator to link directly to dbghelp shared library.
Created attachment 119739 [details] [review] Add x86_64 support to backtrace generator on windows.
Created attachment 119740 [details] [review] Workaround with infinite loop while walking along the stack frames on wine64.
I splitted patch from attachment 119725 [details] [review] into tree parts. Any problem with them ?
Comment on attachment 119737 [details] [review] Add file and line support to backtrace generator on windows. (update1) Review of attachment 119737 [details] [review]: ----------------------------------------------------------------- Looks OK for master. There's a bit of foo() instead of foo () but not really a problem. ::: dbus/dbus-sysdeps-win.c @@ +2550,5 @@ > + HANDLE hProcess, > + DWORD dwAddr, > + PDWORD pdwDisplacement, > + PIMAGEHLP_LINE Line > +))GetProcAddress (hmodDbgHelp, FUNC(SymGetLineFromAddr)); The indentation is still weird here, but I think you delete it in a subsequent patch anyway, so, whatever.
Comment on attachment 119739 [details] [review] Add x86_64 support to backtrace generator on windows. Review of attachment 119739 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +2490,5 @@ > > pSymbol->SizeOfStruct = sizeof(SYMBOL_INFO); > pSymbol->MaxNameLen = MAX_SYM_NAME; > > + if (SymFromAddr (GetCurrentProcess (), sf.AddrPC.Offset, &displacement, pSymbol)) Does this break 32-bit by passing the wrong type for the 3rd argument to SymFromAddr()? Or was it previously overrunning the 4-byte dwDisplacement into IMAGEHLP_LINE, but working anyway because x86 is little-endian and IMAGEHLP_LINE wasn't initialized yet anyway? ... @@ +2495,3 @@ > { > + if (displacement) > + DPRINTF ("%3d %s+0x%llx", i++, pSymbol->Name, displacement); Should be %I64x instead of %llx, I think?
Comment on attachment 119740 [details] [review] Workaround with infinite loop while walking along the stack frames on wine64. Review of attachment 119740 [details] [review]: ----------------------------------------------------------------- OK
(In reply to Simon McVittie from comment #149) > Review of attachment 119725 [details] [review] > > + BYTE buffer[256]; > > I'd prefer this to be of length sizeof (IMAGEHLP_SYMBOL) + some arbitrary > constant, to guarantee that it is strictly larger than IMAGEHLP_SYMBOL. Doesn't seem to have been fixed, although I might have missed it (I'm not 100% sure what the complete patch series is) > > + pSymbol->MaxNameLength = sizeof (buffer) - sizeof (IMAGEHLP_SYMBOL) + 1; > > Should this be sizeof (buffer) - sizeof (IMAGEHLP_SYMBOL) - 1, or > equivalently sizeof (buffer) - (sizeof (IMAGEHLP_SYMBOL) + 1), to allow > space for an IMAGEHLP_SYMBOL structure followed by MaxNameLength bytes > followed by '\0'? Doesn't seem to have been fixed or explained > if SymGetModuleInfo() fails, you won't print a newline You did fix this though.
(In reply to Simon McVittie from comment #158) > > Doesn't seem to have been fixed, although I might have missed it (I'm not > 100% sure what the complete patch series is) ... > Doesn't seem to have been fixed or explained ... > > if SymGetModuleInfo() fails, you won't print a newline > > You did fix this though. Sorry, I did not mention this in the follup patch. All these issues has been fixed with attachment 119739 [details] [review].
(In reply to Ralf Habacker from comment #159) > (In reply to Simon McVittie from comment #158) > > > > Doesn't seem to have been fixed, although I might have missed it (I'm not > > 100% sure what the complete patch series is) > ... > > Doesn't seem to have been fixed or explained > ... > > > if SymGetModuleInfo() fails, you won't print a newline > > > > You did fix this though. > Sorry, I did not mention this in the follup patch. All these issues has been > fixed with attachment 119739 [details] [review] [review]. attachment 119738 [details] [review]
(In reply to Ralf Habacker from comment #160) > (In reply to Ralf Habacker from comment #159) > > (In reply to Simon McVittie from comment #158) > > > > > > Doesn't seem to have been fixed, although I might have missed it (I'm not > > > 100% sure what the complete patch series is) > > ... > > > Doesn't seem to have been fixed or explained > > ... > > > > if SymGetModuleInfo() fails, you won't print a newline > > > > > > You did fix this though. > > Sorry, I did not mention this in the follup patch. All these issues has been > > fixed with attachment 119739 [details] [review] [review] [review]. > attachment 119738 [details] [review] [review] this patch includes also a replace of the deprecated function SymGetSymFromAddr by SymGetAddr https://msdn.microsoft.com/de-de/library/windows/desktop/ms681344%28v=vs.85%29.aspx
Comment on attachment 119737 [details] [review] Add file and line support to backtrace generator on windows. (update1) committed to master; remaining issues are fixed in next commit.
Comment on attachment 119738 [details] [review] Refactor windows backtrace generator to link directly to dbghelp shared library. committed to master with sligthly modified commit message (add term deprecated)
Comment on attachment 119739 [details] [review] Add x86_64 support to backtrace generator on windows. committed with merged in attachment 120075 [details] [review] reviewed by https://bugs.freedesktop.org/show_bug.cgi?id=93069#c55
Comment on attachment 119738 [details] [review] Refactor windows backtrace generator to link directly to dbghelp shared library. committed to master.
Comment on attachment 119740 [details] [review] Workaround with infinite loop while walking along the stack frames on wine64. committed to master
Created attachment 120084 [details] [review] Use dwarf-2 debug symbol format on non msvc windows builds to support file and line numbers display on wine backtraces.
Comment on attachment 119713 [details] [review] Enable debug symbols on wine. superseeded
Comment on attachment 120084 [details] [review] Use dwarf-2 debug symbol format on non msvc windows builds to support file and line numbers display on wine backtraces. Review of attachment 120084 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ +216,5 @@ > +# set compiler debug flags > +# > +set(DEBUG_CFLAGS "-D_DEBUG") > +if(WIN32 AND NOT MSVC) > + set(DEBUG_CFLAGS "${DEBUG_CFLAGS} -gdwarf-2") If I understand correctly, DEBUG (and RELWITHDEBINFO) should already have something like -g from the normal CMake behaviour anyway. What debug symbols does -g produce, if not DWARF 2?
(In reply to Simon McVittie from comment #169) > Comment on attachment 120084 [details] [review] [review] > Use dwarf-2 debug symbol format on non msvc windows builds to support file > and line numbers display on wine backtraces. > > Review of attachment 120084 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: cmake/CMakeLists.txt > @@ +216,5 @@ > > +# set compiler debug flags > > +# > > +set(DEBUG_CFLAGS "-D_DEBUG") > > +if(WIN32 AND NOT MSVC) > > + set(DEBUG_CFLAGS "${DEBUG_CFLAGS} -gdwarf-2") > > If I understand correctly, DEBUG (and RELWITHDEBINFO) should already have > something like -g from the normal CMake behaviour anyway. What debug symbols > does -g produce, if not DWARF 2? Not sure yet, I did not found a direct reference in the mingw32 doc. From the wine source https://github.com/wine-mirror/wine/blob/master/dlls/dbghelp/pe_module.c#L466 and https://github.com/wine-mirror/wine/blob/master/dlls/dbghelp/pe_module.c#L502 it looks that stabs and dwarf are supported, while stabs is searched first (see https://github.com/wine-mirror/wine/blob/master/dlls/dbghelp/pe_module.c#L706) Compiling manual-backtrace -with and without -gdwarf-2 and then tracing with WINEDEBUG=trace+dbghelp bin/manual-backtrace.exe 2>&1 | less returns in both cases: trace:dbghelp:module_new => PE 400000-41e000 L"S:\\ralf\\src\\dbus-1-cmake-cross-x86-build\\bin\\manual-backtrace.exe" trace:dbghelp:pe_load_stabs failed to load the STABS debug info trace:dbghelp:pe_load_dwarf successfully loaded the DWARF debug info so I would say -g creates dwarf symbols, but with another, unsupported, variant.
(In reply to Ralf Habacker from comment #170) > so I would say -g creates dwarf symbols, but with another, unsupported, > variant. For the record: I compiled with -gstabs and got trace:dbghelp:pe_load_stabs successfully loaded the STABS debug info trace:dbghelp:pe_load_dwarf successfully loaded the DWARF debug info The resulting backtrace is identical to the one compiled with -gdwarf-2.
Created attachment 120171 [details] [review] Add linking to dbhelp for autotools on Windows which is required for backtrace generator. Bug:
Comment on attachment 120171 [details] [review] Add linking to dbhelp for autotools on Windows which is required for backtrace generator. Review of attachment 120171 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +1226,4 @@ > if test x$dbus_wince = xyes ; then > NETWORK_libs="-lws2" > else > + NETWORK_libs="-lws2_32 -liphlpapi dbghelp" Beside the fact, that it should be -ldbghelp, it does not work. Any hint where to add this dependency welcome.
Created attachment 120186 [details] [review] Add linking to dbghelp for autotools on Windows which is required for backtrace generator. Bug:
(In reply to Ralf Habacker from comment #173) > Comment on attachment 120171 [details] [review] [review] > it does not work. Any hint > where to add this dependency welcome. Got it - patching configure.ac requires to regenerate configure.
Comment on attachment 120186 [details] [review] Add linking to dbghelp for autotools on Windows which is required for backtrace generator. I've applied this one to master for 1.11.0, since it was breaking the build. I'll look at your various other patches when I can.
(In reply to Simon McVittie from comment #169) > Use dwarf-2 debug symbol format on non msvc windows builds to support file > and line numbers display on wine backtraces. Is this still necessary/desired? (In reply to Ralf Habacker from comment #170) > I would say -g creates dwarf symbols, but with another, unsupported, > variant. Which version of gcc are you using, from which vendor? (mingw.org? mingw-w64?) My gcc-7 manual says that on "most targets", -gdwarf is equivalent to -gdwarf-4. -g can be equivalent to anything (target-dependent) but I would hope that if it's DWARF, the default for -g and -gdwarf should be the same? Please try with explicit -gdwarf-3 or -gdwarf-4? If you get the same results that you get with plain -g, then that would maybe imply that Wine supports DWARF 2 but not those newer versions? I'd consider this patch with a more explanatory commit message, if it's necessary. However, it would perhaps be better to talk to the supplier of your compiler and see whether they'd consider changing the default for -g to a format that Wine can decode?
-- 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/133.
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.