Merge lp:~alan-griffiths/compiz-core/Bug.833729 into lp:compiz-core

[Alan Griffiths]

If composite doesn't load exit the process instead of crashing later

If composite doesn't load, then very few plugins will work (and most will crash). So exit with a clue to help figure out the underlying problem.

[Alan Griffiths (alan-griffiths)]

I agree in advance that this is a horrible hack. But making the client code either handle errors, or exception neutral is a LOT of work. This could give us some more immediate feedback on the causes of failure.

[Łukasz Zemczak (sil2100)]

This is indeed a very hacky approach, but I understand that it's handle it gracefully. But I'm a bit reluctant about using a macro for this like it. It might be confusing later on if not explicitly noted in the comments that this is 'temporary'. Such code I would prefer not to include in compiz-core, but maybe as a distro patch? Or wouldn't it be possible overwrite the setFailed() method for CompositeScreen to do the exit() call and some debug printing? I know we would loose the neat _LINE_ thing, but such an approach would at least be more stylish. Still, even that wouldn't be the final solution IMO. Not sure about what could!

[lp:~alan-griffiths/compiz-core/Bug.833729]

3070. By Alan Griffiths on 2012-03-22

Apology

[Daniel van Vugt (vanvugt)]

Could you please add some more *critical* info to:

            compLogMessage ("composite", CompLogLevelError,
                            "Screen %d on display \"%s\" already "
                            "has a compositing manager; try using the "
                            "--replace option to replace the current "
                            "compositing manager.",
                            screen->screenNum (), DisplayString (dpy));

So that it also outputs the currentCmSnOwner printed in 0xhex format? Then we will at least know which is the offending window.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Also, I'm wondering how hard it would be to check and enforce plugin dependencies in core, so that core doesn't try to load any plugin that depends on one which has failed to load...

That would be better than merging this branch.

[lp:~alan-griffiths/compiz-core/Bug.833729]

3071. By Alan Griffiths on 2012-03-23

More diagnositic information

3072. By Alan Griffiths on 2012-03-23

merge

[Alan Griffiths (alan-griffiths)]

Added requested diagnostic.

I don't see how we can fix plugin loading without an ABI change - too much indulgence in templates.

[Daniel van Vugt (vanvugt)]

Agreed. Any nice fix will break the ABI. Let's keep this.

review: Approve

=== modified file 'plugins/composite/src/screen.cpp'
--- plugins/composite/src/screen.cpp	2012-02-09 06:57:51 +0000
+++ plugins/composite/src/screen.cpp	2012-03-23 09:36:20 +0000
@@ -45,6 +45,9 @@
 
 #include <core/timer.h>
 
+#include <cstdlib>
+#include <cstdio>
+
 CompWindow *lastDamagedWindow = 0;
 
 void
@@ -195,6 +198,26 @@
     return priv->damageEvent;
 }
 
+/* TODO - remove these macros, also <cstdio> and <cstdlib> above.
+ *
+ * We're seeing errors in code that tries to use composite after
+ * it fails to initialise.
+ *
+ * I agree in advance that this macro subverting setFailed() is a
+ * horrible hack.  But making the client code either handle errors,
+ * or exception neutral, is a LOT of work.
+ *
+ * This could give us some more immediate feedback on the causes
+ * of failure and a better solution.
+ *                                             Alan Griffiths
+ */
+#define ARG_STRINGIZE(x) ARG_STRINGIZE_(x)
+#define ARG_STRINGIZE_(x) #x
+#define setFailed()\
+do { \
+    std::fputs("error CompositeScreen::CompositeScreen - line " ARG_STRINGIZE(__LINE__) "\n", stderr);\
+    std::exit(EXIT_FAILURE);\
+} while (false)
 
 CompositeScreen::CompositeScreen (CompScreen *s) :
     PluginClassHandler<CompositeScreen, CompScreen, COMPIZ_COMPOSITE_ABI> (s),
@@ -257,6 +280,11 @@
 
 }
 
+#undef ARG_STRINGIZE
+#undef ARG_STRINGIZE_
+#undef setFailed
+
+
 CompositeScreen::~CompositeScreen ()
 {
     priv->paintTimer.stop ();
@@ -316,12 +344,12 @@
     {
 	if (!replaceCurrentWm)
 	{
-	    compLogMessage ("composite", CompLogLevelError,
-			    "Screen %d on display \"%s\" already "
-			    "has a compositing manager; try using the "
-			    "--replace option to replace the current "
-			    "compositing manager.",
-			    screen->screenNum (), DisplayString (dpy));
+	    compLogMessage (
+		"composite", CompLogLevelError,
+		"Screen %d on display \"%s\" already has a compositing "
+		"manager (%x); try using the --replace option to replace "
+		"the current compositing manager.",
+		screen->screenNum (), DisplayString (dpy), currentCmSnOwner);
 
 	    return false;
 	}