Bug 93464 - [Patch] New multi-threaded message sending test case.
Summary: [Patch] New multi-threaded message sending test case.
Status: RESOLVED MOVED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: x86-64 (AMD64) Linux (All)
: medium enhancement
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review-
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-12-21 06:41 UTC by amit tewari
Modified: 2018-10-12 21:26 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
[Patch] New multi-threaded message sending test case. (9.15 KB, patch)
2015-12-21 06:41 UTC, amit tewari
Details | Splinter Review
Updated patch with removed compile time warnings (9.15 KB, patch)
2016-02-01 10:38 UTC, amit tewari
Details | Splinter Review
[Patch] Updated patch with compilte time warning removed with latest master branch (9.17 KB, patch)
2016-02-01 10:58 UTC, amit tewari
Details | Splinter Review
Updated patch with handled naming convention and pthread prototype (9.20 KB, patch)
2016-02-03 09:19 UTC, amit tewari
Details | Splinter Review
Updated patch with handled coding guideline and pthread prototype (9.26 KB, patch)
2016-02-03 10:57 UTC, amit tewari
Details | Splinter Review

Description amit tewari 2015-12-21 06:41:58 UTC
Created attachment 120621 [details] [review]
[Patch] New multi-threaded message sending test case.

Hello,

We would like to contribute a test case to dbus community for further extending dbus test-coverage.
 
We find that multi-threaded message sending test case is not available in libdbus test case.
So we wish to contribute a test case where N (= 5) threads send message to server in a blocking mode and check the reply.

It will help to improve libdbus test case as well as provide a good reference for libdbus users to use it.

Test case description -

Test name: test-multithread-client

Test Source: dbus/test/name-test/test-multithread-client.c and dbus/test/name-test/test-multithread-server.c

Patch: 0001-Multithread-dbus-server-client-communication-test-ca.patch
Dbus version : 1.11.1 (Master branch)

Description: 

This Test program integrates into name-test directory of dbus master code and runs along with other embedded test cases when enabled in configure option.
Test case working :
 1) Server starts and registers a object path handler and opens session on bus
2) Client connect to dbus and creates 5 threads and sends message using dbus_connection_send_with_reply_and_block() method call.
3) Each thread sends  1MB of data through shared connection.
4) Data is received and reply is sent back to client
5) Reply is verified for thread and ack.

Please review the patch and let us know the response.

Thanks
Amit Tewari
Comment 1 amit tewari 2016-01-21 08:48:21 UTC
Hello,

Please review the patch and let us know the response.

Thanks
Amit Tewari
Comment 2 Ralf Habacker 2016-01-21 10:00:12 UTC
I applied the patch to dbus master, compiled with cmake on linux (which worked out of the box) and did run the server with

bin/test-multithread-server &
bin/test-multithread-client 

and got the line 

Client : Reply received  thread = ...

125 times. 

Is that the expected result ? 

In that case other test print out a dedicated test status 

...:~/src/dbus-cmake-build> bin/test-refs
/refs/connection: OK
/refs/message: OK
/refs/pending-call: OK
/refs/server: OK
Comment 3 amit tewari 2016-01-21 10:03:23 UTC
(In reply to Ralf Habacker from comment #2)
> I applied the patch to dbus master, compiled with cmake on linux (which
> worked out of the box) and did run the server with
> bin/test-multithread-server &
bin/test-multithread-client 

and got the line
> Client : Reply received  thread = ...

125 times. 

Is that the expected
> result ? 

In that case other test print out a dedicated test status 
> ...:~/src/dbus-cmake-build> bin/test-refs
/refs/connection: OK
> /refs/message: OK
/refs/pending-call: OK
/refs/server: OK


Yes actually we are running 5 thread that runs in loop_count=25 to send message to bus and verify the Id in reply received. since we received all reply correctly so we got message and its success case.
Comment 4 amit tewari 2016-01-28 10:55:55 UTC
Hello 

Please let us know your response. 

Thanks
Comment 5 Ralf Habacker 2016-01-28 20:33:54 UTC
(In reply to amit tewari from comment #3)

> Yes actually we are running 5 thread that runs in loop_count=25 to send
> message to bus and verify the Id in reply received. since we received all
> reply correctly so we got message and its success case.

This test should be used to test the recently compiled dbus source ? 
In that case some setup required:
1. start a local dbus-damon 
2. start the server providing the interface to test
3. run the client
4. after test exits terminate server and local dbus-daemin 

This setup is normally done by the dbus test suite, to which this test should be added to be able to be runable by make check. Your patch is missing this integration.
Comment 6 amit tewari 2016-01-29 04:32:18 UTC
(In reply to Ralf Habacker from comment #5)
> (In reply to amit tewari from comment #3)

> Yes actually we are running 5
> thread that runs in loop_count=25 to send
> message to bus and verify the Id
> in reply received. since we received all
> reply correctly so we got message
> and its success case.

This test should be used to test the recently
> compiled dbus source ? 
In that case some setup required:
1. start a local
> dbus-damon 
2. start the server providing the interface to test
3. run the
> client
4. after test exits terminate server and local dbus-daemin 

This
> setup is normally done by the dbus test suite, to which this test should be
> added to be able to be runable by make check. Your patch is missing this
> integration.

I actually tested my testcase integrating into the dbus test/name-test/run-test.sh  script but since it can run only maximum 8 test case and also it can run only the testcases currently added in the run-test.sh script due to which I am not able to add it into dbus testsuite.
 Can you please add this to the name-test/run-test.sh script and then it will do the setup automatically.
Comment 7 Ralf Habacker 2016-02-01 07:43:20 UTC
Comment on attachment 120621 [details] [review]
[Patch] New multi-threaded message sending test case.

Review of attachment 120621 [details] [review]:
-----------------------------------------------------------------

compiling on dbus master with autotools on linux fails

The related build has been configured with --enable-tests

As already mentioned the integration in test/name-test/run-test.sh is missing. 

The number of tests could be increased in run-test.sh at the following fragment
... 
# TAP test plan: we will run 8 tests
echo "1..8"

Please fix the remaining issue.

::: test/name-test/test-multithread-client.c
@@ +18,5 @@
> +  exit (1);
> +}
> +
> +void*
> +send_multithread_data()

error: no previous prototype for ‘send_multithread_data’ [-Werror=missing-prototypes]

@@ +59,5 @@
> +      die("Client: Error getting ARG from Reply");
> +
> +    if(!(i == Ret_Id && ack && !strcmp(ack,"DataRecieved")))
> +      die ("Invalid reply from server");
> +    printf("Client : Reply received  thread = %u\n", pthread_self());

: no previous prototype for ‘send_multithread_data’ [-Werror=missing-prototypes]

@@ +75,5 @@
> +main(int argc, char** argv)
> +{
> +  int i = 0;
> +  dbus_threads_init_default();
> +  pthread_t mythread[MAXTHREAD];

error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
Comment 8 amit tewari 2016-02-01 09:08:26 UTC
(In reply to Ralf Habacker from comment #7)
> Comment on attachment 120621 [details] [review] [review]
[Patch] New multi-threaded
> message sending test case.

Review of attachment 120621 [details] [review] [review]:
> -----------------------------------------------------------------

compiling
> on dbus master with autotools on linux fails

The related build has been
> configured with --enable-tests

As already mentioned the integration in
> test/name-test/run-test.sh is missing. 

The number of tests could be
> increased in run-test.sh at the following fragment
... 
# TAP test plan: we
> will run 8 tests
echo "1..8"

Please fix the remaining issue.

:::
> test/name-test/test-multithread-client.c
@@ +18,5 @@
> +  exit (1);
> +}
> +
> > +void*
> +send_multithread_data()

error: no previous prototype for
> ‘send_multithread_data’ [-Werror=missing-prototypes]

@@ +59,5 @@
> +     
> die("Client: Error getting ARG from Reply");
> +
> +    if(!(i == Ret_Id &&
> ack && !strcmp(ack,"DataRecieved")))
> +      die ("Invalid reply from
> server");
> +    printf("Client : Reply received  thread = %u\n",
> pthread_self());

: no previous prototype for ‘send_multithread_data’
> [-Werror=missing-prototypes]

@@ +75,5 @@
> +main(int argc, char** argv)
>
> +{
> +  int i = 0;
> +  dbus_threads_init_default();
> +  pthread_t
> mythread[MAXTHREAD];

error: ISO C90 forbids mixed declarations and code
> [-Werror=declaration-after-statement]


Can you please let me know how you compile the code to reproduce these errors. I will be following the same to rectify them.

I do add these test cases to run-test.sh in the end script but still it say some errors , 
" ERROR: run-test.sh 9 test-multithread-client # UNPLANNED
ERROR: run-test.sh - too many tests run (expected 8, got 9
"
I added testcase in the end in run-test.sh script
============

# TAP test plan: we will run 8 tests
echo "1..8"

c_test test-ids
c_test test-pending-call-dispatch
c_test test-pending-call-timeout
c_test test-threads-init
c_test test-privserver-client
c_test test-shutdown
py_test test-activation-forking.py
c_test test-autolaunch
c_test test-multithread-client
                                          
======================

I can see name-test testcases run with "--enable-embedded-tests" in configure.
Comment 9 amit tewari 2016-02-01 10:38:28 UTC
Created attachment 121435 [details] [review]
Updated patch with removed compile time warnings

Updated patch with removed compile time warnings
Comment 10 amit tewari 2016-02-01 10:58:17 UTC
Created attachment 121438 [details] [review]
[Patch] Updated patch with compilte time warning removed with latest master branch

This Patch is handling all compile time error and is tested with latest master branch
Comment 11 Ralf Habacker 2016-02-01 11:14:37 UTC
(In reply to amit tewari from comment #8)

> Can you please let me know how you compile the code to reproduce these
> errors. I will be following the same to rectify them.

1. checkout dbus-source from git master branch into <dbus-source-dir>
2. create <dbus-build-dir> and enter it 
3. run <dbus-source-dir>/autogen.sh --enable-tests
4. run make

> 
> I do add these test cases to run-test.sh in the end script but still it say
> some errors , 
> " ERROR: run-test.sh 9 test-multithread-client # UNPLANNED
> ERROR: run-test.sh - too many tests run (expected 8, got 9
> "
> I added testcase in the end in run-test.sh script
> ============
> 
> # TAP test plan: we will run 8 tests
> echo "1..8"

# TAP test plan: we will run 9 tests
echo "1..9"
Comment 12 amit tewari 2016-02-01 11:47:25 UTC
I have updated the patch for all compile time errors.
Can you please test at your end integrating in run-test.sh as I am getting still getting the errors.
Comment 13 Ralf Habacker 2016-02-01 19:00:47 UTC
(In reply to amit tewari from comment #12)
> I have updated the patch for all compile time errors.
> Can you please test at your end integrating in run-test.sh as I am getting
> still getting the errors.

With the following patch 

diff --git a/test/name-test/run-test.sh b/test/name-test/run-test.sh
index 8b9e7ad..8aaef68 100755
--- a/test/name-test/run-test.sh
+++ b/test/name-test/run-test.sh
@@ -67,8 +67,8 @@ py_test () {
 }
 
 test_num=1
-# TAP test plan: we will run 8 tests
-echo "1..8"
+# TAP test plan: we will run 9 tests
+echo "1..9"
 
 c_test test-ids
 c_test test-pending-call-dispatch
@@ -78,3 +78,4 @@ c_test test-privserver-client
 c_test test-shutdown
 py_test test-activation-forking.py
 c_test test-autolaunch
+c_test test-multithread-client


I get 

PASS: run-test.sh 1 test-ids
PASS: run-test.sh 2 test-pending-call-dispatch
PASS: run-test.sh 3 test-pending-call-timeout
PASS: run-test.sh 4 test-threads-init
PASS: run-test.sh 5 test-privserver-client
PASS: run-test.sh 6 test-shutdown
PASS: run-test.sh 7 test-activation-forking.py
PASS: run-test.sh 8 test-autolaunch
PASS: run-test.sh 9 test-multithread-client
PASS: run-test-systemserver.sh 1 test-expected-echo-fail --print-reply --dest=org.freedesktop.DBus.TestSuiteEchoService /org/freedesktop/TestSuite org.freedesktop.TestSuite.Echo string:hi (Saw "DBus.Error" in output as expected)
PASS: run-test-systemserver.sh 2 test-wait-for-echo.py
============================================================================
Testsuite summary for dbus 1.11.1
============================================================================
# TOTAL: 11
# PASS:  11
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
Comment 14 Ralf Habacker 2016-02-01 21:14:14 UTC
Supporting tests in test/name-test on windows/wine depends on the patches of bug 92899
Comment 15 amit tewari 2016-02-02 03:19:55 UTC
(In reply to Ralf Habacker from comment #14)
> Supporting tests in test/name-test on windows/wine depends on the patches of
> bug 92899

Will it be merged in test/name-test directory currently for Linux support.
Thanks
Comment 16 amit tewari 2016-02-03 05:29:50 UTC
I think adding it for Linux support will be good since we have recently fixed run-test.sh script for name-test tests in master branch.
Comment 17 Ralf Habacker 2016-02-03 07:16:01 UTC
Comment on attachment 121438 [details] [review]
[Patch] Updated patch with compilte time warning removed with latest master branch

Review of attachment 121438 [details] [review]:
-----------------------------------------------------------------

Found some remaining name issues, please fix.

::: test/Makefile.am
@@ +324,4 @@
>  	data/valid-service-files/org.freedesktop.DBus.TestSuiteSegfaultService.service.in \
>  	data/valid-service-files/org.freedesktop.DBus.TestSuiteShellEchoServiceFail.service.in \
>  	data/valid-service-files/org.freedesktop.DBus.TestSuiteShellEchoServiceSuccess.service.in \
> +	data/valid-service-files/org.freedesktop.DBus.TestSuite.Multithreaddbus.service.in \

Please use org.freedesktop.DBus.TestSuite.MultiThreadServer.service.in

::: test/data/valid-service-files/org.freedesktop.DBus.TestSuite.Multithreaddbus.service.in
@@ +1,1 @@
> +[D-BUS Service]

filename should be org.freedesktop.DBus.TestSuite.MultiThreadServer.service.in

@@ +1,2 @@
> +[D-BUS Service]
> +Name=org.freedesktop.DBus.TestSuite.Multithreaddbus

Please use  

Name=org.freedesktop.DBus.TestSuite.MultiThreadServer

::: test/name-test/test-multithread-client.c
@@ +1,5 @@
> +#include <config.h>
> +#include<pthread.h>
> +#include "../test-utils.h"
> +
> +#define DBUS_MULTITHREAD_SERVICE "org.freedesktop.DBus.TestSuite.Multithreaddbus"

The most similar service is Name=org.freedesktop.DBus.TestSuite.PrivServer which uses camel case names 

org.freedesktop.DBus.TestSuite.PrivServer

soe it should be 

org.freedesktop.DBus.TestSuite.MultiThreadServer

@@ +2,5 @@
> +#include<pthread.h>
> +#include "../test-utils.h"
> +
> +#define DBUS_MULTITHREAD_SERVICE "org.freedesktop.DBus.TestSuite.Multithreaddbus"
> +#define DBUS_MULTITHREAD_PATH "/org/freedesktop/DBus/TestSuite/Multithreaddbus"

see above

@@ +18,5 @@
> +  exit (1);
> +}
> +
> +static void*
> +send_multithread_data()

wrong solution. 

From http://man7.org/linux/man-pages/man3/pthread_create.3.html the prototype of pthread_create():

pthread_create is defined as: 

int pthread_create(pthread_t *thread, const pthread_attr_t *attr,
                          void *(*start_routine) (void *), void *arg);

where the start routine need to be of type void *start_routine(void *)

::: test/name-test/test-multithread-server.c
@@ +18,5 @@
> +  DBusError error;
> +  char *ack="DataRecieved";
> +  dbus_error_init (&error);
> +  if (dbus_message_is_method_call (msg,
> +				"org.freedesktop.DBus.TestSuite.Multithreaddbus",

please use org.freedesktop.DBus.TestSuite.MultiThreadServer

@@ +19,5 @@
> +  char *ack="DataRecieved";
> +  dbus_error_init (&error);
> +  if (dbus_message_is_method_call (msg,
> +				"org.freedesktop.DBus.TestSuite.Multithreaddbus",
> +				"MultithreadData"))

please use MultiThreadData

@@ +55,5 @@
> +
> +  loop = _dbus_loop_new ();
> +  test_connection_setup (loop, dbus_conn);
> +
> +  dbus_bus_request_name(dbus_conn, "org.freedesktop.DBus.TestSuite.Multithreaddbus", 0, &error);

see above

@@ +66,5 @@
> +  dbus_vtable->unregister_function = NULL;
> +  dbus_vtable->message_function = handler_function;
> +
> +  if(!dbus_connection_register_object_path(dbus_conn,
> +					"/org/freedesktop/DBus/TestSuite/Multithreaddbus",

see above
Comment 18 Ralf Habacker 2016-02-03 07:39:49 UTC
(In reply to amit tewari from comment #16)
> I think adding it for Linux support will be good since we have recently
> fixed run-test.sh script for name-test tests in master branch.

Would be nice, unfortunally the patch in the current state is incomplete because it is missing the make check integration (for autotools: patch fragment from comment 8) and cmake (need some patching)
Comment 19 Ralf Habacker 2016-02-03 07:51:53 UTC
(In reply to Ralf Habacker from comment #18)
> (In reply to amit tewari from comment #16)
> > I think adding it for Linux support will be good since we have recently
> > fixed run-test.sh script for name-test tests in master branch.
> 
> Would be nice, unfortunally the patch in the current state is incomplete
> because it is missing the make check integration (for autotools: patch
> fragment from comment 8) and cmake (need some patching)

see bug 93976 for cmake support running tests in name-test dir.
Comment 20 amit tewari 2016-02-03 09:19:19 UTC
Created attachment 121484 [details] [review]
Updated patch with handled naming convention and pthread prototype
Comment 21 amit tewari 2016-02-03 09:21:52 UTC
(In reply to Ralf Habacker from comment #19)
> (In reply to Ralf Habacker from comment #18)
> (In reply to amit tewari from
> comment #16)
> > I think adding it for Linux support will be good since we
> have recently
> > fixed run-test.sh script for name-test tests in master
> branch.
> 
> Would be nice, unfortunally the patch in the current state is
> incomplete
> because it is missing the make check integration (for
> autotools: patch
> fragment from comment 8) and cmake (need some patching)
> see bug 93976 for cmake support running tests in name-test dir.

I understand now it is important for this bug 93976 to get fixed for this testcase to merge.
but I can run  test cases separately from name-test dir using "make check" in name-test dir, if I run configure using "--enable-embedded-tests" option.
Comment 22 amit tewari 2016-02-03 09:33:04 UTC
I think since the bug 93976 changes will apply to all testcases within name-test dir, we can merge this testcase to name-test dir so that once the blocking bug is closed multithread testcase will also get included and run along with other name-test testcases, this way multithread testcase will be preserved for other developers also.
Thanks
Comment 23 Ralf Habacker 2016-02-03 10:25:22 UTC
Comment on attachment 121484 [details] [review]
Updated patch with handled naming convention and pthread prototype

Review of attachment 121484 [details] [review]:
-----------------------------------------------------------------

::: test/name-test/test-multithread-client.c
@@ +18,5 @@
> +  exit (1);
> +}
> +
> +static int
> +send_multithread_data()

still an issue http://man7.org/linux/man-pages/man3/pthread_create.3.html stated to use the following prototype style

   static void *thread_start (void *arg)

whichs means to change the function name to 

static void *
send_multithread_data (void *args)

Please note that dbus uses <function-name><space>( style; there are additional locations in this file affected.

@@ +71,5 @@
> +
> +}
> +
> +int
> +main(int argc, char** argv)

space after function name missing

@@ +78,5 @@
> +  pthread_t mythread[MAXTHREAD];
> +  dbus_threads_init_default();
> +  for(i = 0; i < MAXTHREAD; i++)
> +  {
> +    if(pthread_create(&mythread[i],NULL,(void *)send_multithread_data,NULL)!=0)

no cast, use type safe &send_multithread_data

::: test/name-test/test-multithread-server.c
@@ +11,5 @@
> +  exit (1);
> +}
> +
> +static DBusHandlerResult
> +handler_function(DBusConnection *conn, DBusMessage *msg, void *user_data)

space after function name missing, please fix also the additional related locations in this file.
Comment 24 amit tewari 2016-02-03 10:57:52 UTC
Created attachment 121487 [details] [review]
Updated patch with handled coding guideline and pthread prototype
Comment 25 Simon McVittie 2016-02-03 12:14:52 UTC
Comment on attachment 121487 [details] [review]
Updated patch with handled coding guideline and pthread prototype

Review of attachment 121487 [details] [review]:
-----------------------------------------------------------------

For the commit message: it's spelled "D-Bus". "dbus" is the name of the reference implementation of D-Bus; "Dbus" doesn't exist.

General high-level questions and points about these:

Why is this particular test interesting? You're sending some arbitrary number of messages, in a blocking (synchronous) way, from multiple sender threads, to a recipient out-of-process. Did this exhibit a bug for you? Are you using some other code for which this is a simplified emulation? Is there some particular piece of code that gets coverage this way but not in any of the existing tests? What makes this better than any of the million other possible tests?

I would prefer new tests to start their own dbus-daemon, like test/dbus-daemon.c does, instead of running in name-test/ and relying on a separate dbus-daemon activating an out-of-process service.

I would prefer new tests to do their messaging by "talking to themselves" - running the service side in-process, like test/dbus-daemon.c does, instead of in a separate activated process like this one does - because that makes it *much* easier to run both client and service sides under a debugger like gdb, or a QA tool like valgrind or helgrind.

I would prefer new tests to use GTest, like test/dbus-daemon does; or failing that, output in TAP format <http://testanything.org/>.

If you are adding new tests to name-test/ despite those considerations, please edit run-test.sh to enable them. There is nothing magic about the line that says "1..8": it's a TAP <http://testanything.org/> "test plan". Patching it to say "1..9" would be fine. I don't want to add new tests that have not been run in the test environment by their authors, and in general I don't want to add new tests that will not be run automatically by the test framework, unless there's a very good reason why that can't be done (for instance needing manual setup, like test/manual-authz.c, or manual interpretation of the results, like test/manual-paths.c).

::: test/name-test/test-multithread-client.c
@@ +1,1 @@
> +#include <config.h>

New code needs a copyright statement and a license: either the same dual-license as dbus/dbus-connection.c (GPL-2+|AFL-2.1), or the same permissive license as test/dbus-daemon.c (the same MIT/X11 license as Expat). Either is fine; for new modules like this one I'd prefer the permissive license, so that it can be copied into other projects and/or used as an example.

@@ +1,2 @@
> +#include <config.h>
> +#include<pthread.h>

This is weird syntax, please do the conventional spacing.

@@ +3,5 @@
> +#include "../test-utils.h"
> +
> +#define DBUS_MULTITHREAD_SERVICE "org.freedesktop.DBus.TestSuite.MultiThreadServer"
> +#define DBUS_MULTITHREAD_PATH "/org/freedesktop/DBus/TestSuite/MultiThreadServer"
> +#define DBUS_MULTITHREAD_IFACE DBUS_MULTITHREAD_SERVICE

Please don't use the DBUS/dbus/DBus namespace for things that are not API. These constants could be called MULTITHREAD_WHATEVER or TEST_MULTITHREAD_WHATEVER.

@@ +5,5 @@
> +#define DBUS_MULTITHREAD_SERVICE "org.freedesktop.DBus.TestSuite.MultiThreadServer"
> +#define DBUS_MULTITHREAD_PATH "/org/freedesktop/DBus/TestSuite/MultiThreadServer"
> +#define DBUS_MULTITHREAD_IFACE DBUS_MULTITHREAD_SERVICE
> +#define MAXTHREAD 5
> +pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;

If you're going to hard-code use of pthreads (as opposed to using some threading abstraction like the one from GLib), then this file needs to be conditionally added to lists of executables in both CMake and Autotools build systems, so that it's only compiled on Unix systems (or equivalently, isn't compiled for Windows, where pthread is not a standard platform API, and we use native Win32 thread support).

@@ +15,5 @@
> +  va_start (args, message);
> +  vfprintf (stderr, message, args);
> +  va_end (args);
> +  exit (1);
> +}

I'd prefer this to add a "\n" automatically, so that calls to die() don't have to remember to include one (which you have not done consistently).

@@ +25,5 @@
> +  DBusConnection *dbus_conn;
> +  DBusMessage *reply;
> +  DBusError error;
> +
> +  const long long DataSize =  1024*1024;

Please prefer to use portable types: size_t for memory sizes, something like dbus_uint64_t for general large integers.

Please use D-Bus' usual naming conventions: variables_are_lower_case_with_underscores, TypesAreCapitalized, CONSTANTS_ARE_UPPER_CASE_WITH_UNDERSCORES, and nothing uses Both_Caps_And_Underscores. For instance this should be data_size.

@@ +33,5 @@
> +  char *ack;
> +  dbus_error_init (&error);
> +  dbus_conn =  dbus_bus_get (DBUS_BUS_SESSION, &error);
> +  if (dbus_error_is_set (&error))
> +    die ("Client:couldn't access session bus\n");

This should print the name and human-readable message from 'error'.

@@ +36,5 @@
> +  if (dbus_error_is_set (&error))
> +    die ("Client:couldn't access session bus\n");
> +
> +  for (i = 0 ; i < Num_Msg ; i++ )
> +  {

Please indent all new code in GNU style: {} are indented 2 spaces further than the "for", "if", etc., and the contents of {} are indented 2 more spaces. For instance see test/dbus-daemon.c, which is correctly-indented.

@@ +37,5 @@
> +    die ("Client:couldn't access session bus\n");
> +
> +  for (i = 0 ; i < Num_Msg ; i++ )
> +  {
> +    char *data = (char*)malloc (DataSize);

This cast is unnecessary: ISO C allows void * to be assigned to char * without a cast.

Please prefer to use dbus_malloc() and similar when writing for libdbus.

@@ +40,5 @@
> +  {
> +    char *data = (char*)malloc (DataSize);
> +    if (!data)
> +      die ("couldn't malloc data\n");
> +    memset (data, 'a', DataSize*sizeof(char));

data_size * sizeof (char)

@@ +51,5 @@
> +    dbus_message_append_args (message, DBUS_TYPE_INT32, &i, DBUS_TYPE_STRING, &data, DBUS_TYPE_INVALID);
> +    reply = dbus_connection_send_with_reply_and_block (dbus_conn, message, -1, &error);
> +    if (dbus_error_is_set (&error))
> +    {  die ("Client:couldn't send message: %s\n", error.message);
> +      dbus_message_unref (message);

This die() call is particularly weirdly indented. It should print the error name and message. I'm pretty sure test-utils has a convenience function to assert that a DBusError is unset...

@@ +55,5 @@
> +      dbus_message_unref (message);
> +    }
> +
> +    if (!dbus_message_get_args (reply, &error, DBUS_TYPE_INT32, &Ret_Id, DBUS_TYPE_STRING, &ack, DBUS_TYPE_INVALID))
> +      die ("Client: Error getting ARG from Reply");

This should print the error name and message.

@@ +58,5 @@
> +    if (!dbus_message_get_args (reply, &error, DBUS_TYPE_INT32, &Ret_Id, DBUS_TYPE_STRING, &ack, DBUS_TYPE_INVALID))
> +      die ("Client: Error getting ARG from Reply");
> +
> +    if (!(i == Ret_Id && ack && !strcmp (ack, "DataRecieved")))
> +      die ("Invalid reply from server");

If you're going to include magic strings in your protocol, please spell "received" correctly :-)

@@ +59,5 @@
> +      die ("Client: Error getting ARG from Reply");
> +
> +    if (!(i == Ret_Id && ack && !strcmp (ack, "DataRecieved")))
> +      die ("Invalid reply from server");
> +    printf ("Client : Reply received  thread = %lu\n", (unsigned long)pthread_self ());

Tests should not output junk on stdout: I'm trying to move our tests towards TAP format, which parses stdout. If you need to output progress indications, use stderr.

Printing to stdout or stderr from non-main threads seems very questionable: at best you'll get randomly interleaved lines, and at worst you'll make all your threads' operations effectively serialized (because they're waiting for the lock that libc presumably has around each FILE * to avoid crashes), removing the value of testing multi-threaded operation.

@@ +61,5 @@
> +    if (!(i == Ret_Id && ack && !strcmp (ack, "DataRecieved")))
> +      die ("Invalid reply from server");
> +    printf ("Client : Reply received  thread = %lu\n", (unsigned long)pthread_self ());
> +
> +    if (reply)

if (reply != NULL)

@@ +74,5 @@
> +int
> +main (int argc, char** argv)
> +{
> +  int i = 0;
> +  pthread_t mythread[MAXTHREAD];

I'd prefer a blank line between declarations and code, to make it more obvious that we're working in what is basically C89 (no interleaving declarations with code).

@@ +75,5 @@
> +main (int argc, char** argv)
> +{
> +  int i = 0;
> +  pthread_t mythread[MAXTHREAD];
> +  dbus_threads_init_default ();

This should not have been necessary since 1.8.

@@ +79,5 @@
> +  dbus_threads_init_default ();
> +  for (i = 0; i < MAXTHREAD; i++)
> +  {
> +    if (pthread_create (&mythread[i], NULL, send_multithread_data, NULL) != 0)
> +      die ("error creating thread");

This is an example of forgetting to put the \n on the die() call.

This should presumably mention strerror() somewhere.

::: test/name-test/test-multithread-server.c
@@ +45,5 @@
> +  DBusObjectPathVTable *dbus_vtable;
> +  DBusError error;
> +  DBusLoop *loop;
> +
> +  dbus_threads_init_default ();

This hasn't been necessary since 1.8.

@@ +49,5 @@
> +  dbus_threads_init_default ();
> +  dbus_error_init (&error);
> +
> +  dbus_conn =  dbus_bus_get (DBUS_BUS_SESSION, &error);
> +  if(dbus_error_is_set (&error))

if (...

@@ +61,5 @@
> +    die ("couldn't request name: %s", error.message);
> +
> +  dbus_vtable = malloc (sizeof(DBusObjectPathVTable));
> +  if (!dbus_vtable)
> +    die ("error malloc vtable");

This doesn't need to be malloc()'d, it could be on the stack.
Comment 26 Simon McVittie 2016-02-03 12:16:14 UTC
(In reply to amit tewari from comment #21)
> I understand now it is important for this bug 93976 to get fixed for this
> testcase to merge.

That is not required.
Comment 27 GitLab Migration User 2018-10-12 21:26:06 UTC
-- 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/138.


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.