[Patch] Use Qt XML to decode config.xml files

[Michael Sprauer]

 On Mon, 4 Apr 2011 15:00:28 +0100, Simon McVittie 
 <simon.mcvittie at collabora.co.uk> wrote:
> On Mon, 04 Apr 2011 at 15:14:54 +0200, Michael Sprauer wrote:
>> I wrote a patch to support QTs XML parsers to decode the config XML
>> in order to be not depended to an aditional external library.
>
> Please open a bug with severity "enhancement"?
>
> I'm not sure that this avoiding a library dependence is sufficient 
> reason to
> support this, since it brings a few problems:
>
> * the libxml2 backend is already under-maintained/unmaintained and 
> might not
>   work; the Expat backend is the only one that's really tested
>
> * since QtXML and QtDBus are released together, using Qt to parse XML 
> gives
>   us a circular dependency: Qt contains QtDBus needs libdbus which
> needs QtXML
>   which is part of Qt
>
> but if you're sure this is needed, perhaps other D-Bus maintainers 
> could be
> convinced.

 I'll post that "enhancement"...

>
>> Other (related) fixes:
>> Fix to FindQt4.cmake: Find Qt4 properly
>> dbus-bus.c: Applied the patch for windows (makes dbus-win.patch
>> superfluous)
>> dbus-message.c: avoid compiler warning
>
> Please open one or more bugs for these; otherwise they'll get lost. I 
> think
> these three should all be considered separately, so, separate bugs 
> please.
>
> Some drive-by reviews:
>
>> Subject: [PATCH] Added XML load using Qt4 XML library instead of 
>> libxml2 or expat
>>  Fix to FindQt4.cmake: Find Qt4 properly
>>  dbus-bus.c: Applied patch for windows
>>  dbus-message.c: avoid compiler warning
>
> This should be at least four git commits, since it makes at least 
> four
> unrelated changes: if you find yourself writing "bullet points" in 
> your
> commit message, it's usually a sign that you should have committed 
> several
> patches.
 OK.
>
>> diff --git a/bus/config-loader-QXml.cpp b/bus/config-loader-QXml.cpp
>> new file mode 100644
>> index 0000000..a520138
>> --- /dev/null
>> +++ b/bus/config-loader-QXml.cpp
>
> This is missing a copyright statement; please use the same license 
> and style
> as the rest of the dbus code.
 OK.
>
>> @@ -0,0 +1,191 @@
>> +#include "vld.h"
>
> What's vld.h and is it portable?
 My mistake. This should be removed. It was an attempt to detect the 
 memory leaks...
>
>> +extern "C" void free_attributes(char **attribute_names, char 
>> **attribute_values, int size)
>> +{
>> +    for(int i=0;i<size;i++)
>> +    {
>> +                     if (attribute_names[i] != NULL)
>> +        dbus_free((void*)attribute_names[i]);
>
> This isn't anywhere near being in D-Bus indentation/spacing style; 
> could you
> clean it up, please?
>
> Since this file is necessarily Qt-specific, I wouldn't necessarily 
> object if
> you wanted to format it in kdelibs style
> (<http://techbase.kde.org/Policies/Kdelibs_Coding_Style>) instead of 
> in
> D-Bus style.
 Done.
>
>> diff --git a/cmake/CMakeLists.txt b/cmake/CMakeLists.txt
>> index e623e2d..70149e5 100644
>> --- a/cmake/CMakeLists.txt
>> +++ b/cmake/CMakeLists.txt
>> @@ -40,6 +40,10 @@ find_package(LibXml2)
>>  find_package(LibExpat)
>>  find_package(X11)
>>
>> +SET(QT_DONT_USE_QTGUI true)
>> +SET(QT_USE_QTXML true)
>> +find_package(Qt4 REQUIRED )
>
> This should only require QtXml if using Qt as the XML backend, 
> surely?
 True. Fixed...
>
>> Fix to FindQt4.cmake: Find Qt4 properly
>
> Do you actually mean "update from some third-party location"? If you 
> do,
> please say where you got it from, and ideally also which revision 
> number or
> commit ID you're updating to.
>
> Or, if these changes are your own work, please explain in the body of 
> the
> commit message what was wrong with the previous code or how you fixed 
> it.
 That came out of my head. I added QtXmlPatterns and thought of using 
 it.

 And I found the other line somewhere with google, while I was searching 
 for the correct way to find qt. To source was something like this: 
 http://quarkplayer.googlecode.com/svn-history/r1387/trunk/cmake/modules/FindQt4.cmake

>
>> diff --git a/dbus/dbus-bus.c b/dbus/dbus-bus.c
>> index 92ec20e..a59afe1 100644
>> --- a/dbus/dbus-bus.c
>> +++ b/dbus/dbus-bus.c
>> @@ -402,8 +402,10 @@ 
>> _dbus_bus_notify_shared_connection_disconnected_unlocked 
>> (DBusConnection *connec
>>  {
>>    int i;
>>
>> +#if !defined(DBUS_WIN) && !defined(DBUS_WINCE)
>> +   // qt example pong says "QMutex::lock: Deadlock detected"
>>    _DBUS_LOCK (bus);
>> -
>> +#endif
>
> Either the code in this function should be protected by a lock, or it
> shouldn't; there should be no such thing as "locked, but only if 
> you're
> on Windows".
>
> If you don't know why you're getting that warning, please open a bug 
> with a
> backtrace from each thread, so we can work out which pair of locks 
> are
> deadlocking each other.
>
> That function is called with the DBusConnection's lock held, so I
> suspect that
> the answer is that the dbus-bus lock and the DBusConnection lock can 
> be taken
> in either order. That would be a D-Bus bug; you're just hiding the
> warning that
> illustrates that a bug exists.

 This did not come out of my head. It seems to be necessary as it is in 
 the patch file: 
 http://cgit.freedesktop.org/dbus/dbus/commit/?id=51e88a912a893ffc45298cc0ddca3b556193570f
 I just applied the patch. Don't know if this is needed?!

>
>>  static DBusConnection *
>> diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
>> index d053559..11a2eb1 100644
>> --- a/dbus/dbus-message.c
>> +++ b/dbus/dbus-message.c
>> @@ -3839,6 +3839,7 @@ 
>> _dbus_message_loader_get_unix_fds(DBusMessageLoader  *loader,
>>    return TRUE;
>>  #else
>>    _dbus_assert_not_reached("Platform doesn't support unix fd 
>> passing");
>> +  return FALSE;
>>  #endif
>
> This will probably *cause* warnings under gcc, because
> _dbus_assert_not_reached can't return (like abort() or exit()). The
> "return FALSE" can't actually be reached, and gcc knows that via the
> _DBUS_GNUC_NORETURN annotation.
>
> According to some Googling, it seems MSVC++ can support this via
> __declspec(noreturn). Does __declspec(noreturn) have to appear like 
> this:
>
>     __declspec(noreturn) void exit (int code);
>
> or can it be used like this?
>
>     void exit (int code) __declspec(noreturn);
>
> If it can be used after the declaration, we can define 
> _DBUS_GNUC_NORETURN
> to __declspec(noreturn) on MSVC++; if it has to be used before, we 
> can
> probably still do that, but we'd have to move it to the beginning of 
> the
> declaration everywhere it's used.

 I'll investigate this.


 Thank you for your detailed answer.

 Michael