Should fix QTBUG-186 and QTBUG-5066

I need automatic unit tests (in tests/auto).

I will not look at this until there are tests there and some of the current functionality is tested in peer-to-peer mode.

No one looked at this merge request for over two months, and now the “missing tests” are somehow used as an excuse for that?

Right, some general style bits from me now I finally remembered to take a look at this…

First, you appear to be missing copyright/license information at the top of your tests. I'm not positive this is required for those, but I'd assume it is.

tests/manual/qdbuspeertopeer/server/peer-server.cpp:

you have some lines commented out – if they aren’t needed, remove them.. keeping them there is unnecessarily messy

tests/manual/qdbuspeertopeer/server/myobject.h: (and .cpp)

casing of your methods is probably wrong – I've never seen methods start with uppercase anywhere else in Qt

run_test.sh… this is full of what looks to be vestigial bits(?) – also, how is this going to fit in with test automation? I don’t see it being run from anywhere

tests/manual/qdbuspeertopeer/client/peer-client.cpp:

// TODO Check read error

.. self explanatory I think? :)

shouldn’t be too hard to fix, just QVERIFY() that data was read, no?

also: it might be an idea to QVERIFY() the QFile::open() call to make the test fail if the open fails (though I imagine that to be very unlikely, it’s still nice to make life easy when it happens)

Last comment for now.. you need a \since 4.8 in the qdoc for newly added methods.

There’s some more problems in similar vein to what I've outlined here, so I won’t repeat myself.. I’ll re-review when you've updated. :)

I updated the patch:

Changelog:
Reverted most of the QDBusServer changes because they were wrong.
Fixed some more small bugs discovered using unit tests.
Split the merge request into smaller and more significant commits
Added a “disconnectFromPeer()” method in QDBusConnection
Added unit tests.
Applied some changes suggested by Robin Burchell

Please note that:

• The first commit is just some cleaning, but it is useful to keep the single commits more readable, you can discard it if necessary

• All commits except the 2 adding “connectToPeer” and “disconnectFromPeer” methods don’t make any change to the API, so they could be backported. This also applies to the commit fixing QDBusServer, so you could backport it and have a working server also in previous version. Of course you cannot connect to that using Qt without the connectToPeer method

• The last commit adds a test in test/manual that is supposed to be useful to demonstrate that the connection works also with client/server not using Qt, that’s why the name of the interface is “org.freedesktop.DBus.GLib.Tests.MyObject” and the methods start with uppercase. If you don’t need this just disregard this commit.

Okay. Full review from me:

src/dbus/qdbusconnection.cpp

- line 403: if it isn't needed, remove it please

- I'm admittedly not great with threading, but:  
    QDBusConnection::disconnectFromBus:  
      your addition in here touches _q_manager() before creating the QMutexLocker..  
      I presume the rest of the code creates the locker first for a reason

    QDBusConnection::disconnectFromPeer:  
      also touches _q_manager() before a QMutexLocker (line 415)

src/dbus/qdbusintegrator.cpp

- line 369: unnecessary whitespace change here (yes, I'm nitpicky :-))

- line 422: wrong coding style (leading brace should be on the same line, not a line of its own)

- line 1006: might as well clean up the wrong style here.  
  a single line if statement doesn't require braces unless it is fairly complex (or has an else if/else)

- lines 1600/1617: more unnecessary whitespace changes

- QDBusServer::newConnection documentation:  
  typically, Qt docs try work the parameter name into the docs itself.. perhaps something like:   
   This signal is emitted when a new client \a connection is established to the server

tests/manual/qdbuspeertopeer/client/myobjectclient.cpp:

- remove commented out debug (lines 52, 61)

- //TODO check timeout  
  (please do it, affects testDoNothing, testIncrement, testEmitFrobnicate, testTerminate)

- line 122: brace should not be on a line of its own  

- line 124: please use QTest::qSleep() instead of an OS-dependent sleep()

- (by the way, is there a reason frobnicateEmitted is a uint instead of a regular int?)

tests/manual/qdbuspeertopeer/client/peer-client.cpp:

- remove commented out debug, line 49
- still lacking checking of the read error, too  
- what is the commented out QTimer stuff for? remove it if it's not needed  
- why do you call app.processEvents() and straight after app.exec()? just remove the processEvents call

tests/manual/qdbuspeertopeer/run_test.sh:

- you clarified on IRC what it is for (manual running of tests)
  I'd still either leave a comment to that effect in the file or remov  
  it (given how simple it looks..)

tests/manual/qdbuspeertopeer/server/myobject.cpp:

- remove the commented out return, collapse the if () statement per coding guidelines as so:
    if (!connection.registerObject(@/@, m_obj))  
        qDebug("connection.registerObject: FAIL");  

tests/manual/qdbuspeertopeer/server/peer-server.cpp:

- what is the app.processEvents() call doing there?
  looks like it might be required for the server.address() print below. is that the case? if so, might want to comment it

Hello Robin!
Thanks again for your review!

src/dbus/qdbusconnection.cpp

- line 403: if it isn't needed, remove it please

you mean remove the QDBusConnection::connectToPeer method?
That would be great because it could avoid api changes, but from my point of view it is nonsense… It could be possible to do the prefix hack suggested by oggis_ on IRC to modify connectToBus to accept addresses of the form __FOOINVALIDP2PSOCKETPREFIXunix://whatever, but it is really ugly and we would be using a method to do something that is not what the method is supposed to do.

- I'm admittedly not great with threading, but:
    QDBusConnection::disconnectFromBus:  
      your addition in here touches _q_manager() before creating the QMutexLocker..  
      I presume the rest of the code creates the locker first for a reason  

    QDBusConnection::disconnectFromPeer:  
      also touches _q_manager() before a QMutexLocker (line 415)

I don’t know, I must definitely check those!
Thanks for pointing them out.

src/dbus/qdbusintegrator.cpp

- line 369: unnecessary whitespace change here (yes, I'm nitpicky :-))

- lines 1600/1617: more unnecessary whitespace changes

As I tried to explain, the whitespace and all the “unnecessary” code-cleaning changes are in a separate commit, if necessary I could just remove that commit, but it looks like a wrong style to me, so I just fixed it…

- line 422: wrong coding style (leading brace should be on the same line, not a line of its own)

- line 1006: might as well clean up the wrong style here.  
  a single line if statement doesn't require braces unless it is fairly complex (or has an else if/else)

I will fix those in next version

- QDBusServer::newConnection documentation:
  typically, Qt docs try work the parameter name into the docs itself.. perhaps something like:   
   This signal is emitted when a new client \a connection is established to the server

I don’t understand this (maybe it’s some little nuance that I can’t get in English, sorry) what’s wrong with this?

This signal is emitted when a new client is connected to the server
\a connection is the new connection.

(Ok there is at least a “.” missing ;) )

tests/manual/qdbuspeertopeer/client/myobjectclient.cpp:
tests/manual/qdbuspeertopeer/client/peer-client.cpp:
tests/manual/qdbuspeertopeer/run_test.sh:
tests/manual/qdbuspeertopeer/server/myobject.cpp:
tests/manual/qdbuspeertopeer/server/peer-server.cpp:

Those are all about the “manual” test, I think I should just remove it as it is not needed… What do you think about?

I'm afraid I have no idea how to reply to comments on gitorious (…sigh..) so I’ll just bold your issues and add my comments.

you mean remove the QDBusConnection::connectToPeer method?

Sorry, I got the wrong line number. I meant the commented out Q_ASSERT_X, line 406. Personally, I think the API distinction is a good one given that they are really supposed to do different things. :)

As I tried to explain, the whitespace and all the “unnecessary” code-cleaning changes are in a separate commit, if necessary I could just remove that commit, but it looks like a wrong style to me, so I just fixed it…

OK, I missed that when skimming over, and also didn’t realise that was a seperate commit. So that’s OK by me.

I don’t understand this (maybe it’s some little nuance that I can’t get in English, sorry) what’s wrong with this?

Taking a look at an example of a random Qt method from QtDBus:

247 /*!
248 Creates a QDBusConnection object attached to the connection with name \a name.

Note how the parameter documentation (\a) is inline in the actual text body, and not separate, as in your added docs? That’s what I meant. :)

WRT the manual test.. (I didn’t notice it was manual, doh :)) does it test anything that the automatic test doesn’t cover? Personally, I'd merge it all into the autotest if at all possible: 99% of the time, you can bet that people won’t run them, whereas /tests/auto/ are run before integration, so you can guarantee that people won’t screw things up.

I'm afraid I have no idea how to reply to comments on gitorious
(…sigh..) so I’ll just bold your issues and add my comments.

Just insert a “> ” at the beginning of the line ;)

you mean remove the QDBusConnection::connectToPeer method?

Sorry, I got the wrong line number. I meant the commented out Q_ASSERT_X, line 406.

The first part of the QDBusConnection::connectToPeer method is quite similar to QDBusConnection::connectToBus, and the commented Q_ASSERT_X is also in that method, so I thought that there could be a reason why it is there…

Note how the parameter documentation (\a) is inline in the actual text body, and not separate, as in your added docs? That’s what I meant. :)

Ok, I’ll change it to:

This signal is emitted when a new client connection \a connection is established to the server.

WRT the manual test.. (I didn’t notice it was manual, doh :)) does it test anything that the automatic test doesn’t cover? Personally, I'd merge it all into the autotest if at all possible: 99% of the time, you can bet that people won’t run them, whereas /tests/auto/ are run before integration, so you can guarantee that people won’t screw things up.

I wrote this test because I wanted to ensure that a QtDBusServer could be used with a glib dbus connection and that a glib dbus server could be used with a QtDBusConnection.
I think that the only “missing” test in automatic tests is a “cross library” test, I really have no idea if this is needed, if it could be useful for testers just to test the patch and then discarded, or if is just useless…

So, reviewing as well, of course skipping already existing issues pointed out by Robin. To me it looks very good, but being very, very, very picky, there’s still small room for improvement, in those 2 points:

• Maybe you should opt for better commit names: “Fix QDBusServer”, for example, isn’t really explainatory to me.

• QDBusServer, line 76 (1st commit): Actually, this change is pointless and unneeded: QObject will normalize the slot and signal’s signature anyway, so you definitely don’t need to add const & to the signature at all in connect(). Please revert this change and watch over your connect() in the rest of the patch.

Skipping some other comments regarding the manual test, as you told me on IRC it will go away in the next revision. Overall, address Robin comments and this is gold quality to me!

\o/

Any estimate on when this stuff could become officially available?

Changelog Version 6:

• Removed manual test.
  
• Rebased on updated origin/master

Changes suggested by Robin Burchell:

• Creating the QMutexLocker before touching _q_manager()
  
• Fixed style (src/dbus/qdbusintegrator.cpp lines 422 and 1006)
  
• Fixed documentation for QDBusServer::newConnection

Changes suggested by Dario Freddi:

• Changed commit changelof from “Fix QDBusServer” to “Fix QDBusServer to handle correctly new dbus connections”
  
• Reverted change to connect()

Again I suggest to merge this patch to master and cherrypick the first 6 commits to stable branches

Thanks for your changes. For what concerns me, this patch is ready for merge and I have no further comments on it.

@Michael: It actually depends on Qt people, but hopefully soon – Thiago asked for some help in reviewing it, so that’s why I chimed in :)

Something looks screwed with the update here. I see a whole lot of mess, and none of your work.

Did you do a merge instead of a rebase or something? Can you try again?

Something looks screwed with the update here. I see a whole lot of mess, and none of your work.

Did you do a merge instead of a rebase or something? Can you try again?

¹?!?&$

I think that gitorious had some problem, it added several versions (also the previous version was the #2 but it was marked as #5).
Probably it adds a version also when the description for a merge request is changed, but it is not shown immediately.
I pushed the last version again…

I'm getting a merge conflict when trying to apply this onto latest master.

diff —cc src/dbus/qdbusintegrator.cpp
index 35979d6,3ef625f..0000000
—– a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@@ -2062,29 -2051,32 +2061,58 @@@ void QDBusConnectionPrivate::connectSig

  if (connection) {
      qDBusDebug("Adding rule: %s", hook.matchRule.constData());  

++<<<<<<< HEAD
+ QDBusErrorInternal error;
+ q_dbus_bus_add_match(connection, hook.matchRule, error);
+ if (!!error) {
+ QDBusError qerror = error;
+ qWarning(“QDBusConnectionPrivate::connectSignal: received error from D-Bus server ”
+ “while connecting signal to %s::%s: %s (%s)”,
+ hook.obj->metaObject()–>className(),
+ hook.obj->metaObject()–>method(hook.midx).signature(),
+ qPrintable(qerror.name()), qPrintable(qerror.message()));
+ Q_ASSERT(false);
+ } else {
+ // Successfully connected the signal
+ // Do we need to watch for this name?
+ if (shouldWatchService(hook.service)) {
+ WatchedServicesHash::mapped_type &data = watchedServices[hook.service];
+ if (++data.refcount == 1) {
+ // we need to watch for this service changing
+ connectSignal(dbusServiceString(), QString(), dbusInterfaceString(),
+ QLatin1String(“NameOwnerChanged”), QStringList() << hook.service, QString(),
+ this, SLOT(q_serviceOwnerChanged(QString,QString,QString)));
+ data.owner = getNameOwnerNoCache(hook.service);
+ qDBusDebug() << this << “Watching service” << hook.service << “for owner changes (current owner:”
+ << data.owner << “)”;
++=======
+ if (mode != QDBusConnectionPrivate::PeerMode) {
+ QDBusErrorInternal error;
+ q_dbus_bus_add_match(connection, hook.matchRule, error);
+ if (!!error) {
+ QDBusError qerror = error;
+ qWarning(“QDBusConnectionPrivate::connectSignal: received error from D-Bus server ”
+ “while connecting signal to %s::%s: %s (%s)”,
+ hook.obj->metaObject()–>className(),
+ hook.obj->metaObject()–>method(hook.midx).signature(),
+ qPrintable(qerror.name()), qPrintable(qerror.message()));
+ Q_ASSERT(false);
+ } else {
+ // Successfully connected the signal
+ // Do we need to watch for this name?
+ if (shouldWatchService(hook.service)) {
+ WatchedServicesHash::mapped_type &data = watchedServices[hook.service];
+ if (++data.refcount == 1) {
+ // we need to watch for this service changing
+ QString dbusServerService = QLatin1String(DBUS_SERVICE_DBUS);
+ connectSignal(dbusServerService, QString(), QLatin1String(DBUS_INTERFACE_DBUS),
+ QLatin1String(“NameOwnerChanged”), QStringList() << hook.service, QString(),
+ this, SLOT(q_serviceOwnerChanged(QString,QString,QString)));
+ data.owner = getNameOwnerNoCache(hook.service);
+ qDBusDebug() << this << “Watching service” << hook.service << “for owner changes (current owner:”
+ << data.owner << “)”;
+ }
++>>>>>>> 3e6c6929c735873069de701f6b697c13f470c26f

              }
          }  
      }

Fixed in version 12

Sent to code scan

Commit 4 (QtDBus: Fix empty service name in peer-to-peer connections) requires a testcase. I don’t know what it’s trying to fix.

Commit 5 (QtDBus: Fix empty service name in peer-to-peer connections) I understand, but it also needs a testcase.

Commit 6 (QtDBus: Fix bus in peer-to-peer connections should not be used) makes sense: you don’t send AddMatch and RemoveMatch for non-bus connections. It’s missing the same modification for the RemoveMatch side of things.

There are minor coding style issues (missing spaces after if, in “){”, etc.)

Suggestion: QDBusServer should have a constructor with a default address of “unix:tmpdir=/tmp”.

Please fix the issues highlighted by Thiago. The testcase coverage is critical.

NOTE: Just add the testcases as a new commit. No need to rebase it into the current commits, as we've already scanned those with Legal.

Commit 4 (QtDBus: Fix empty service name in peer-to-peer connections) requires a testcase. I don’t know what it’s trying to fix.

Commit 5 (QtDBus: Fix empty service name in peer-to-peer connections) I understand, but it also needs a testcase.

You mixed up some commit numbers and logs, so I don’t understand which comment are you referring to.

Commit 4 (QtDBus: Fix empty service name in peer-to-peer connections) is needed because the method QDBusAbstractInterfacePrivate::canMakeCalls() checks if the service is empty and always returns false for dbus p2p connection. This method is called before making each call and the call is not executed if the return value is false. Service name have no meaning for p2p connection, so this check is useless and blocks calls. This is why I think it should be skipped for p2p dbus connection.
In my opinion, being able to actually make calls in p2p dbus connections is already a testcase for this commit, so the tests already cover it, if you don’t agree please clarify what do you want me to test. Also I don’t know how to test a private class, so any hint is welcome.

Changelog version 13:

• Rebased onto recent master because commit 5 (QtDBus: Fix bus in peer-to-peer connections should not be used) failed to merge due to a recent change. Commit 5 is the only commit changed (also because required to be fixed), the other commits are unmodified (2 new commits added).
  
• Fix commit 5 (QtDBus: Fix bus in peer-to-peer connections should not be used)
  
  • debug line (#2063) moved inside the if statement, so that it’s not printed if rule is not added

Changes suggested by Thiago Macieira

• Fix minor coding style issues (new commit)
  
• Add default constructor to QDBusServer (new commit)
  
• Fix commit 5 (QtDBus: Fix bus in peer-to-peer connections should not be used)
  
  • doing the same for RemoveMatch

Just for information, I spoke with thiago on IRC a few days ago and will write the requested unit tests, but I'm too busy to do it now, I will do it in a couple of weeks.

Changelog version 15:

• Some more minor coding style issues and cleaning
  
• Skip bus name check for peer-to-peer connection
  
• Register QDBusServer connection name in QDBusConnectionManager
  
• Add unit tests for QDBusAbstractInterface
  
• Add unit tests for QDBusInterface
  
• Add unit tests for QDBusAbstractAdaptor

(I Forgot this)

• Rebased onto recent master because commit 5 (QtDBus: Fix bus in peer-to-peer connections should not be used) failed to merge again due to another recent change. Commit 5 is the only commit changed (from Marius Storm-Olsen’s comment)

Gitorious is broken, now it gives this error: “This Git object is too large to be displayed in the browser Consider cloning the repository locally and look at the object there”, so I cannot check if I made mistakes or if gitorious is doing weird things, I tried to clone it and everything seemed ok, than It switched back to the previous version after I edited the merge request, so I pushed again and now I cannot clone it any more for some reason that goes beyond my understanding.

If You manage to clone, this is the list of the commits, please check that it is correct

• 363692f QtDBus: Add unit tests for QDBusAbstractAdaptor
  
• 7ebf6c6 QtDBus: Add unit tests for QDBusInterface
  
• dc439c8 QtDBus: Add unit tests for QDBusAbstractInterface
  
• 182e65e QtDBus: Register QDBusServer connection name in QDBusConnectionManager
  
• 4aa5694 QtDBus: Skip bus name check for peer-to-peer connection
  
• ad7ab3a QtDBus: Fix minor coding style issues
  
• 9e52afa QtDBus: Add default constructor to QDBusServer
  
• 11d3c7c QtDBus: Add unit tests for peer-to-peer connection
  
• 7f1c249 QtDBus: Add method QDBusConnection::disconnectFromPeer()
  
• 57fa246 QtDBus: Add method QDBusConnection::connectToPeer()
  
• 71e9a98 QtDBus: Fix QDBusConnection::disconnectFromBus() for peer-to-peer connections
  
• de25fb0 QtDBus: Fix bus in peer-to-peer connections should not be used
  
• 9220c10 QtDBus: Fix empty service name in peer-to-peer connections
  
• 1add34c QtDBus: Fix registering objects using path ‘/’ in peer-to-peer connections
  
• d15abd5 QtDBus: Fix QDBusServer to handle correctly new dbus connections
  
• 41dba2a QtDBus: Cleaning comments, spacing, etc.

This is the right list of commits, sorry about the previous…
Anyway now it seems to work!

• 4f30048 QtDBus: Add unit tests for QDBusAbstractAdaptor
  
• cbeac52 QtDBus: Add unit tests for QDBusInterface
  
• cfa6f3b QtDBus: Add unit tests for QDBusAbstractInterface
  
• 2046cf6 QtDBus: Register QDBusServer connection name in QDBusConnectionManager
  
• 2f35922 QtDBus: Skip bus name check for peer-to-peer connection
  
• 6d9d272 QtDBus: Fix minor coding style issues
  
• 02de6fb QtDBus: Add default constructor to QDBusServer
  
• cae92bf QtDBus: Add unit tests for peer-to-peer connection
  
• 4dfbcfc QtDBus: Add method QDBusConnection::disconnectFromPeer()
  
• 1f03abd QtDBus: Add method QDBusConnection::connectToPeer()
  
• d81d905 QtDBus: Fix QDBusConnection::disconnectFromBus() for peer-to-peer connections
  
• 270274c QtDBus: Fix bus in peer-to-peer connections should not be used
  
• 9220c10 QtDBus: Fix empty service name in peer-to-peer connections
  
• 1add34c QtDBus: Fix registering objects using path ‘/’ in peer-to-peer connections
  
• d15abd5 QtDBus: Fix QDBusServer to handle correctly new dbus connections
  
• 41dba2a QtDBus: Cleaning comments, spacing, etc.

It’s back to the previous version!
I'm really bored, this is last push, then I give up

Thanks!!!

commit 685df07ff7e357f6848c50cffa311641afdca307
Merge: a359d21 4c8c5ef
Author: Qt Continuous Integration System qt-info@nokia.com
Date: Tue Apr 19 08:17:02 2011 +1000

Merge branch 'master' of scm.dev.nokia.troll.no:qt/oslo-staging-1 into master-integration

* 'master' of scm.dev.nokia.troll.no:qt/oslo-staging-1:  
  QtDBus: Add unit tests for QDBusAbstractAdaptor  
  QtDBus: Add unit tests for QDBusInterface  
  QtDBus: Add unit tests for QDBusAbstractInterface  
  QtDBus: Register QDBusServer connection name in QDBusConnectionManager  
  QtDBus: Skip bus name check for peer-to-peer connection  
  QtDBus: Fix minor coding style issues  
  QtDBus: Add default constructor to QDBusServer  
  QtDBus: Add unit tests for peer-to-peer connection  
  QtDBus: Add method QDBusConnection::disconnectFromPeer()  
  QtDBus: Add method QDBusConnection::connectToPeer()  
  QtDBus: Fix QDBusConnection::disconnectFromBus() for peer-to-peer connections  
  QtDBus: Fix bus in peer-to-peer connections should not be used  
  QtDBus: Fix empty service name in peer-to-peer connections  
  QtDBus: Fix registering objects using path '/' in peer-to-peer connections  
  QtDBus: Fix QDBusServer to handle correctly new dbus connections  
  QtDBus: Cleaning comments, spacing, etc.