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

[Simon McVittie]

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.

> 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.

> 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.

> @@ -0,0 +1,191 @@
> +#include "vld.h"

What's vld.h and is it portable?

> +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.

> 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?

> 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.

> 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.

>  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.

    S