Adapt the message-sending code to return error messages from the

marshalling code.

Reviewed-By: Harald Fernengel

4 files changed, 104 insertions, 51 deletions

diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index 76179c9..704d2e3 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -1144,9 +1144,12 @@ void QDBusConnectionPrivate::relaySignal(QObject *obj, const QMetaObject *mo, in
     QDBusMessage message = QDBusMessage::createSignal(QLatin1String("/"), interface,
                                                       QLatin1String(memberName));
     message.setArguments(args);
-    DBusMessage *msg = QDBusMessagePrivate::toDBusMessage(message);
+    QDBusError error;
+    DBusMessage *msg = QDBusMessagePrivate::toDBusMessage(message, &error);
     if (!msg) {
-        qWarning("QDBusConnection: Could not emit signal %s.%s", qPrintable(interface), memberName.constData());
+        qWarning("QDBusConnection: Could not emit signal %s.%s: %s", qPrintable(interface), memberName.constData(),
+                 qPrintable(error.message()));
+        lastError = error;
         return;
     }
 
@@ -1698,21 +1701,26 @@ int QDBusConnectionPrivate::send(const QDBusMessage& message)
         return -1;              // don't send; the reply will be retrieved by the caller
                                 // through the d_ptr->localReply link
 
-    DBusMessage *msg = QDBusMessagePrivate::toDBusMessage(message);
+    QDBusError error;
+    DBusMessage *msg = QDBusMessagePrivate::toDBusMessage(message, &error);
     if (!msg) {
         if (message.type() == QDBusMessage::MethodCallMessage)
-            qWarning("QDBusConnection: error: could not send message to service \"%s\" path \"%s\" interface \"%s\" member \"%s\"",
+            qWarning("QDBusConnection: error: could not send message to service \"%s\" path \"%s\" interface \"%s\" member \"%s\": %s",
                      qPrintable(message.service()), qPrintable(message.path()),
-                     qPrintable(message.interface()), qPrintable(message.member()));
+                     qPrintable(message.interface()), qPrintable(message.member()),
+                     qPrintable(error.message()));
         else if (message.type() == QDBusMessage::SignalMessage)
-            qWarning("QDBusConnection: error: could not send signal path \"%s\" interface \"%s\" member \"%s\"",
+            qWarning("QDBusConnection: error: could not send signal path \"%s\" interface \"%s\" member \"%s\": %s",
                      qPrintable(message.path()), qPrintable(message.interface()),
-                     qPrintable(message.member()));
+                     qPrintable(message.member()),
+                     qPrintable(error.message()));
         else
-            qWarning("QDBusConnection: error: could not send %s message to service \"%s\"",
+            qWarning("QDBusConnection: error: could not send %s message to service \"%s\": %s",
                      message.type() == QDBusMessage::ReplyMessage ? "reply" :
                      message.type() == QDBusMessage::ErrorMessage ? "error" :
-                     "invalid", qPrintable(message.service()));
+                     "invalid", qPrintable(message.service()),
+                     qPrintable(error.message()));
+        lastError = error;
         return 0;
     }
 
@@ -1739,12 +1747,15 @@ QDBusMessage QDBusConnectionPrivate::sendWithReply(const QDBusMessage &message,
         return sendWithReplyLocal(message);
 
     if (!QCoreApplication::instance() || sendMode == QDBus::Block) {
-        DBusMessage *msg = QDBusMessagePrivate::toDBusMessage(message);
+        QDBusError err;
+        DBusMessage *msg = QDBusMessagePrivate::toDBusMessage(message, &err);
         if (!msg) {
-            qWarning("QDBusConnection: error: could not send message to service \"%s\" path \"%s\" interface \"%s\" member \"%s\"",
+            qWarning("QDBusConnection: error: could not send message to service \"%s\" path \"%s\" interface \"%s\" member \"%s\": %s",
                      qPrintable(message.service()), qPrintable(message.path()),
-                     qPrintable(message.interface()), qPrintable(message.member()));
-            return QDBusMessage();
+                     qPrintable(message.interface()), qPrintable(message.member()),
+                     qPrintable(err.message()));
+            lastError = err;
+            return QDBusMessage::createError(err);
         }
 
         qDBusDebug() << QThread::currentThread() << "sending message (blocking):" << message;
@@ -1754,9 +1765,8 @@ QDBusMessage QDBusConnectionPrivate::sendWithReply(const QDBusMessage &message,
         q_dbus_message_unref(msg);
 
         if (!!error) {
-            QDBusError qe = error;
-            lastError = qe;
-            return QDBusMessage::createError(qe);
+            lastError = err = error;
+            return QDBusMessage::createError(err);
         }
 
         QDBusMessage amsg = QDBusMessagePrivate::fromDBusMessage(reply);
@@ -1766,16 +1776,17 @@ QDBusMessage QDBusConnectionPrivate::sendWithReply(const QDBusMessage &message,
         return amsg;
     } else { // use the event loop
         QDBusPendingCallPrivate *pcall = sendWithReplyAsync(message, timeout);
-        if (!pcall)
-            return QDBusMessage();
+        Q_ASSERT(pcall);
 
-        pcall->watcherHelper = new QDBusPendingCallWatcherHelper;
-        QEventLoop loop;
-        loop.connect(pcall->watcherHelper, SIGNAL(reply(QDBusMessage)), SLOT(quit()));
-        loop.connect(pcall->watcherHelper, SIGNAL(error(QDBusError,QDBusMessage)), SLOT(quit())); 
+        if (pcall->replyMessage.type() != QDBusMessage::InvalidMessage) {
+            pcall->watcherHelper = new QDBusPendingCallWatcherHelper;
+            QEventLoop loop;
+            loop.connect(pcall->watcherHelper, SIGNAL(reply(QDBusMessage)), SLOT(quit()));
+            loop.connect(pcall->watcherHelper, SIGNAL(error(QDBusError,QDBusMessage)), SLOT(quit()));
 
-        // enter the event loop and wait for a reply
-        loop.exec(QEventLoop::ExcludeUserInputEvents | QEventLoop::WaitForMoreEvents);
+            // enter the event loop and wait for a reply
+            loop.exec(QEventLoop::ExcludeUserInputEvents | QEventLoop::WaitForMoreEvents);
+        }
 
         QDBusMessage reply = pcall->replyMessage;
         lastError = reply;      // set or clear error
@@ -1831,20 +1842,25 @@ QDBusPendingCallPrivate *QDBusConnectionPrivate::sendWithReplyAsync(const QDBusM
         return pcall;
     }
 
-    DBusMessage *msg = QDBusMessagePrivate::toDBusMessage(message);
+    checkThread();
+    QDBusPendingCallPrivate *pcall = new QDBusPendingCallPrivate;
+    pcall->sentMessage = message;
+    pcall->ref = 0;
+
+    QDBusError error;
+    DBusMessage *msg = QDBusMessagePrivate::toDBusMessage(message, &error);
     if (!msg) {
-        qWarning("QDBusConnection: error: could not send message to service \"%s\" path \"%s\" interface \"%s\" member \"%s\"",
+        qWarning("QDBusConnection: error: could not send message to service \"%s\" path \"%s\" interface \"%s\" member \"%s\": %s",
                  qPrintable(message.service()), qPrintable(message.path()),
-                 qPrintable(message.interface()), qPrintable(message.member()));
-        return 0;
+                 qPrintable(message.interface()), qPrintable(message.member()),
+                 qPrintable(error.message()));
+        pcall->replyMessage = QDBusMessage::createError(error);
+        lastError = error;
+        return pcall;
     }
 
-    checkThread();
     qDBusDebug() << QThread::currentThread() << "sending message (async):" << message;
     DBusPendingCall *pending = 0;
-    QDBusPendingCallPrivate *pcall = new QDBusPendingCallPrivate;
-    pcall->sentMessage = message;
-    pcall->ref = 0;
 
     QDBusDispatchLocker locker(SendWithReplyAsyncAction, this);
     if (q_dbus_connection_send_with_reply(connection, msg, &pending, timeout)) {
@@ -1858,14 +1874,14 @@ QDBusPendingCallPrivate *QDBusConnectionPrivate::sendWithReplyAsync(const QDBusM
             return pcall;
         } else {
             // we're probably disconnected at this point
-            lastError = QDBusError(QDBusError::Disconnected, QLatin1String("Not connected to server"));
+            lastError = error = QDBusError(QDBusError::Disconnected, QLatin1String("Not connected to server"));
         }
     } else {
-        lastError = QDBusError(QDBusError::NoMemory, QLatin1String("Out of memory"));
+        lastError = error = QDBusError(QDBusError::NoMemory, QLatin1String("Out of memory"));
     }
 
     q_dbus_message_unref(msg);
-    pcall->replyMessage = QDBusMessage::createError(lastError);
+    pcall->replyMessage = QDBusMessage::createError(error);
     return pcall;
 }
 
@@ -1874,8 +1890,7 @@ int QDBusConnectionPrivate::sendWithReplyAsync(const QDBusMessage &message, QObj
                                                int timeout)
 {
     QDBusPendingCallPrivate *pcall = sendWithReplyAsync(message, timeout);
-    if (!pcall)
-        return 0;
+    Q_ASSERT(pcall);
 
     // has it already finished (dispatched locally)?
     if (pcall->replyMessage.type() == QDBusMessage::ReplyMessage) {
diff --git a/src/dbus/qdbusmessage.cpp b/src/dbus/qdbusmessage.cpp
index 9150295..eb09de9 100644
--- a/src/dbus/qdbusmessage.cpp
+++ b/src/dbus/qdbusmessage.cpp
@@ -94,11 +94,17 @@ QString QDBusMessage::errorMessage() const
     \internal
     Constructs a DBusMessage object from this object. The returned value must be de-referenced
     with q_dbus_message_unref.
+
+    The \a error object is set to indicate the error if anything went wrong with the
+    marshalling. Usually, this error message will be placed in the reply, as if the call failed.
+    The \a error pointer must not be null.
 */
-DBusMessage *QDBusMessagePrivate::toDBusMessage(const QDBusMessage &message)
+DBusMessage *QDBusMessagePrivate::toDBusMessage(const QDBusMessage &message, QDBusError *error)
 {
-    if (!qdbus_loadLibDBus())
+    if (!qdbus_loadLibDBus()) {
+        *error = QDBusError(QDBusError::Failed, QLatin1String("Could not open lidbus-1 library"));
         return 0;
+    }
 
     DBusMessage *msg = 0;
     const QDBusMessagePrivate *d_ptr = message.d_ptr;
@@ -108,10 +114,17 @@ DBusMessage *QDBusMessagePrivate::toDBusMessage(const QDBusMessage &message)
         //qDebug() << "QDBusMessagePrivate::toDBusMessage" <<  "message is invalid";
         break;
     case DBUS_MESSAGE_TYPE_METHOD_CALL:
-        // only interface can be empty
-        if (d_ptr->service.isEmpty() || d_ptr->path.isEmpty() || d_ptr->name.isEmpty())
-            break;
-        msg = q_dbus_message_new_method_call(d_ptr->service.toUtf8(), d_ptr->path.toUtf8(),
+        // only service and interface can be empty -> path and name must not be empty
+        if (!QDBusUtil::checkBusName(d_ptr->service, QDBusUtil::EmptyAllowed, error))
+            return 0;
+        if (!QDBusUtil::checkObjectPath(d_ptr->path, QDBusUtil::EmptyNotAllowed, error))
+            return 0;
+        if (!QDBusUtil::checkInterfaceName(d_ptr->interface, QDBusUtil::EmptyAllowed, error))
+            return 0;
+        if (!QDBusUtil::checkMemberName(d_ptr->name, QDBusUtil::EmptyNotAllowed, error, "method"))
+            return 0;
+
+        msg = q_dbus_message_new_method_call(data(d_ptr->service.toUtf8()), d_ptr->path.toUtf8(),
                                              data(d_ptr->interface.toUtf8()), d_ptr->name.toUtf8());
         break;
     case DBUS_MESSAGE_TYPE_METHOD_RETURN:
@@ -123,8 +136,9 @@ DBusMessage *QDBusMessagePrivate::toDBusMessage(const QDBusMessage &message)
         break;
     case DBUS_MESSAGE_TYPE_ERROR:
         // error name can't be empty
-        if (d_ptr->name.isEmpty())
-            break;
+        if (!QDBusUtil::checkErrorName(d_ptr->name, QDBusUtil::EmptyNotAllowed, error))
+            return 0;
+
         msg = q_dbus_message_new(DBUS_MESSAGE_TYPE_ERROR);
         q_dbus_message_set_error_name(msg, d_ptr->name.toUtf8());
         if (!d_ptr->localMessage) {
@@ -134,8 +148,13 @@ DBusMessage *QDBusMessagePrivate::toDBusMessage(const QDBusMessage &message)
         break;
     case DBUS_MESSAGE_TYPE_SIGNAL:
         // nothing can be empty here
-        if (d_ptr->path.isEmpty() || d_ptr->interface.isEmpty() || d_ptr->name.isEmpty())
-            break;
+        if (!QDBusUtil::checkObjectPath(d_ptr->path, QDBusUtil::EmptyNotAllowed, error))
+            return 0;
+        if (!QDBusUtil::checkInterfaceName(d_ptr->interface, QDBusUtil::EmptyAllowed, error))
+            return 0;
+        if (!QDBusUtil::checkMemberName(d_ptr->name, QDBusUtil::EmptyNotAllowed, error, "method"))
+            return 0;
+
         msg = q_dbus_message_new_signal(d_ptr->path.toUtf8(), d_ptr->interface.toUtf8(),
                                         d_ptr->name.toUtf8());
         break;
@@ -170,6 +189,7 @@ DBusMessage *QDBusMessagePrivate::toDBusMessage(const QDBusMessage &message)
 
     // not ok;
     q_dbus_message_unref(msg);
+    *error = QDBusError(QDBusError::Failed, QLatin1String("Marshalling failed: ") + marshaller.errorString);
     return 0;
 }
 
@@ -247,7 +267,13 @@ QDBusMessage QDBusMessagePrivate::makeLocal(const QDBusConnectionPrivate &conn,
             // yes, we are
             // we must marshall and demarshall again so as to create QDBusArgument
             // entries for the complex types
-            DBusMessage *message = toDBusMessage(asSent);
+            QDBusError error;
+            DBusMessage *message = toDBusMessage(asSent, &error);
+            if (!message) {
+                // failed to marshall, so it's a call error
+                return QDBusMessage::createError(error);
+            }
+
             q_dbus_message_set_sender(message, conn.baseService.toUtf8());
 
             QDBusMessage retval = fromDBusMessage(message);
diff --git a/src/dbus/qdbusmessage_p.h b/src/dbus/qdbusmessage_p.h
index 12a9500..a0a681f 100644
--- a/src/dbus/qdbusmessage_p.h
+++ b/src/dbus/qdbusmessage_p.h
@@ -80,7 +80,7 @@ public:
     mutable uint delayedReply : 1;
     uint localMessage : 1;
 
-    static DBusMessage *toDBusMessage(const QDBusMessage &message);
+    static DBusMessage *toDBusMessage(const QDBusMessage &message, QDBusError *error);
     static QDBusMessage fromDBusMessage(DBusMessage *dmsg);
 
     static bool isLocal(const QDBusMessage &msg);
diff --git a/tests/auto/qdbusmarshall/tst_qdbusmarshall.cpp b/tests/auto/qdbusmarshall/tst_qdbusmarshall.cpp
index e5b2ebb..3ba789a 100644
--- a/tests/auto/qdbusmarshall/tst_qdbusmarshall.cpp
+++ b/tests/auto/qdbusmarshall/tst_qdbusmarshall.cpp
@@ -793,7 +793,7 @@ void tst_QDBusMarshall::sendErrors()
                                                   "signalName");
     msg << qVariantFromValue(QDBusObjectPath());
 
-    QTest::ignoreMessage(QtWarningMsg, "QDBusConnection: error: could not send signal path \"/foo\" interface \"local.interfaceName\" member \"signalName\"");
+    QTest::ignoreMessage(QtWarningMsg, "QDBusConnection: error: could not send signal path \"/foo\" interface \"local.interfaceName\" member \"signalName\": Marshalling failed: Invalid object path passed in arguments");
     QVERIFY(!con.send(msg));
 
     msg.setArguments(QVariantList());
@@ -803,7 +803,19 @@ void tst_QDBusMarshall::sendErrors()
     path.setPath("abc");
     msg << qVariantFromValue(path);
 
-    QTest::ignoreMessage(QtWarningMsg, "QDBusConnection: error: could not send signal path \"/foo\" interface \"local.interfaceName\" member \"signalName\"");
+    QTest::ignoreMessage(QtWarningMsg, "QDBusConnection: error: could not send signal path \"/foo\" interface \"local.interfaceName\" member \"signalName\": Marshalling failed: Invalid object path passed in arguments");
+    QVERIFY(!con.send(msg));
+
+    QDBusSignature sig;
+    msg.setArguments(QVariantList() << qVariantFromValue(sig));
+    QTest::ignoreMessage(QtWarningMsg, "QDBusConnection: error: could not send signal path \"/foo\" interface \"local.interfaceName\" member \"signalName\": Marshalling failed: Invalid signature passed in arguments");
+    QVERIFY(!con.send(msg));
+
+    QTest::ignoreMessage(QtWarningMsg, "QDBusSignature: invalid signature \"a\"");
+    sig.setSignature("a");
+    msg.setArguments(QVariantList());
+    msg << qVariantFromValue(sig);
+    QTest::ignoreMessage(QtWarningMsg, "QDBusConnection: error: could not send signal path \"/foo\" interface \"local.interfaceName\" member \"signalName\": Marshalling failed: Invalid signature passed in arguments");
     QVERIFY(!con.send(msg));
 }