Query systemd-logind to obtain idle state on Linux

[marcosfrm]

Closes CauldronDevelopmentLLC/cbang#110

Help to implement added TODO welcome.

[marcosfrm]

Bus opening is earlier. Now it might be possible implement PowerManagement::allowSystemSleep() using logind too. @kbernhagen please check if macOS part is ok.

[kbernhagen]

I think you should leave in the memset zero. It covers future vars without a code change.

The osx var init to zero should be harmless.

If the sd bus open can ever fail temporarily, an on demand open would be better because it can retry.

[kbernhagen]

Singleton has a deallocate() method. So maybe add that destructor back with other cleanup.

Maybe:

PowerManagement::~PowerManagement() {
  allowDisplaySleep(true);
  allowSystemSleep(true);
  if (pri) {
#ifdef HAVE_SYSTEMD
    if (pri->bus) sd_bus_flush_close_unref(pri->bus);
#endif
    delete pri;
  }
}

Then client can, I think, call this before exit:

  PowerManagement::instance().deallocate();

[marcosfrm]

If the sd bus open can ever fail temporarily, an on demand open would be better because it can retry.

The idea is to use pri->bus also in PowerManagement::allowSystemSleep(). It has to initialize right away, before PowerManagement::updateIdleSeconds(). Connection failure is very unlikely.

I could not make deallocate() work. Note: sd_bus_flush_close_unref() checks for non-NULL bus internally.

[kbernhagen]

In v7, many objects had a shutdown() method. Maybe use that instead of destructor? It would have to called once-only.

[kbernhagen]

PowerManagement::~PowerManagement() {
  shutdown();
  delete pri;
}

void PowerManagement::shutdown() {
  allowDisplaySleep(true);
  allowSystemSleep(true);
#ifdef HAVE_SYSTEMD
  sd_bus_flush_close_unref(pri->bus);
  pri->bus = NULL;
#endif
}

[kbernhagen]

Maybe not delete pri, because that could cause a null dereference on macOS if the allow methods are called again.

[marcosfrm]

I gave up on sd_bus_flush_close_unref(). If anyone can add it later please do so. On client process exit, D-Bus will detect the peer is gone anyway... (NameLost and NameOwnerChanged signals are emmited).

[kbernhagen]

What’s wrong with the shutdown() method? The client can call that before App::run() returns.

[marcosfrm]

Client:

diff --git a/src/fah/client/App.cpp b/src/fah/client/App.cpp
index b9f355e..7a22463 100644
--- a/src/fah/client/App.cpp
+++ b/src/fah/client/App.cpp
@@ -52,6 +52,7 @@
 #include <cbang/os/SystemInfo.h>
 #include <cbang/os/SignalManager.h> // For SIGHUP on Windows
 #include <cbang/os/CPUInfo.h>
+#include <cbang/os/PowerManagement.h>

 #include <cbang/openssl/SSL.h>
 #include <cbang/openssl/SSLContext.h>
@@ -434,6 +435,8 @@ void App::run() {
   // Event loop
   os->dispatch();

+  PowerManagement::instance().shutdown();
+
   LOG_INFO(1, "Clean exit");
 }

[kbernhagen]

Is the overhead minimal for the client?

Or is the config check useful before getting idle seconds?

bool LinOSImpl::isSystemIdle() const {
  if(!app.getConfig().getOnIdle()) return false;
  return 15 < PowerManagement::instance().getIdleSeconds();
}

[marcosfrm]

This seems cleaner:

diff --git a/src/fah/client/OS.cpp b/src/fah/client/OS.cpp
index 941d45a..1fadd8f 100644
--- a/src/fah/client/OS.cpp
+++ b/src/fah/client/OS.cpp
@@ -29,6 +29,7 @@
 #include "OS.h"

 #include <fah/client/App.h>
+#include <fah/client/Config.h>

 #if defined(_WIN32)
 #include "win/WinOSImpl.h"
@@ -90,7 +91,7 @@ void OS::togglePause() const {

 void OS::update() {
-  if (isSystemIdle() != idle) {
+  if (app.getConfig().getOnIdle() && isSystemIdle() != idle) {
     idle = !idle;
     app.triggerUpdate();
   }

[kbernhagen]

I think that can do the wrong thing depending on previous idle value.

[kbernhagen]

How about

void OS::update() {
-  if (isSystemIdle() != idle) {
+  bool shouldBeIdle = app.getConfig().getOnIdle() && isSystemIdle();
+  if (shouldBeIdle != idle) {
     idle = !idle;
     app.triggerUpdate();
   }

[kbernhagen]

I guess that's the same, but clearer to me.

I misinterpreted the operator precedence.

[kbernhagen]

Does this exist?

#include <cbang/config.h>

[kbernhagen]

cbang/Application.cpp has

#ifdef DEBUG_LEAKS
  // Deallocate singletons
  SingletonDealloc::instance().deallocate();
#endif // DEBUG_LEAKS

So I think a destructor should be defined.

PowerManagement::~PowerManagement() {
  shutdown();
  delete pri;
}

[marcosfrm]

I guess that's the same, but clearer to me.

I prefer keep it as it is now.

Does this exist?

https://github.com/CauldronDevelopmentLLC/cbang/blob/2a995745e0415560346e2a0a4f994a3db2a24b19/SConstruct#L100-L101

HAVE_SYSTEMD will be defined (or not) there.

If DEBUG_LEAKS is defined, client crashes with SIGSEGV on exit (without ~PowerManagement() too). Not sure if the destructor will be useful...

[kbernhagen]

Ah. Thanks for the explanation.

[kbernhagen]

Is this PR done and ready to merge?

[marcosfrm]

diff --git a/config/cbang/__init__.py b/config/cbang/__init__.py
index 5e2ec120..c00921be 100644
--- a/config/cbang/__init__.py
+++ b/config/cbang/__init__.py
@@ -70,6 +70,11 @@ def configure_deps(conf, local = True, with_openssl = True,
             raise SCons.Errors.StopError('Need CoreServices, IOKit, Security '
                                          '& CoreFoundation frameworks')

+    # sd-bus
+    if env['PLATFORM'] == 'posix' and conf.CBCheckCHeader('systemd/sd-bus.h') \
+            and conf.CBCheckLib('systemd'):
+        env.CBConfigDef('HAVE_SYSTEMD')
+
     conf.CBConfig('valgrind', False)

     # Debug
diff --git a/src/cbang/os/PowerManagement.cpp b/src/cbang/os/PowerManagement.cpp
index 9d61a6a5..02ae8fbb 100644
--- a/src/cbang/os/PowerManagement.cpp
+++ b/src/cbang/os/PowerManagement.cpp
@@ -30,12 +30,12 @@
 \******************************************************************************/

 #include "PowerManagement.h"
-#include "DynamicLibrary.h"

 #include <cbang/Exception.h>
 #include <cbang/os/SystemUtilities.h>
 #include <cbang/Catch.h>
 #include <cbang/time/Time.h>
+#include <cbang/Zap.h>

 #if defined(_WIN32)
 #define WIN32_LEAN_AND_MEAN // Avoid including winsock.h
@@ -49,20 +49,9 @@
 #include <IOKit/pwr_mgt/IOPMLib.h>

 #else
-typedef unsigned Window;
-typedef struct {
-  Window window;
-  int state;
-  int kind;
-  unsigned long til_or_since;
-  unsigned long idle;
-  unsigned long eventMask;
-} XScreenSaverInfo;
-
-typedef void *(*XOpenDisplay_t)(char *);
-typedef XScreenSaverInfo *(*XScreenSaverAllocInfo_t)();
-typedef void (*XScreenSaverQueryInfo_t)(void *, unsigned, XScreenSaverInfo *);
-typedef Window (*XDefaultRootWindow_t)(void *);
+#ifdef HAVE_SYSTEMD
+#include <systemd/sd-bus.h>
+#endif
 #endif

 #include <cstring>
@@ -77,20 +66,31 @@ struct PowerManagement::private_t {
   IOPMAssertionID systemAssertionID;
 #elif defined(_WIN32)
 #else
-  bool initialized;
-  void *display;
-  Window root;
-  DynamicLibrary *xssLib;
-  XScreenSaverInfo *info;
+#ifdef HAVE_SYSTEMD
+  sd_bus *bus;
+#endif
 #endif
 };

-PowerManagement::PowerManagement(Inaccessible) :
+PowerManagement::PowerManagement() :
   lastBatteryUpdate(0), systemOnBattery(false), systemHasBattery(false),
   lastIdleSecondsUpdate(0), idleSeconds(0), systemSleepAllowed(true),
   displaySleepAllowed(true), pri(new private_t) {
   memset(pri, 0, sizeof(private_t));
+#ifdef HAVE_SYSTEMD
+  if (sd_bus_open_system(&pri->bus) < 0) pri->bus = NULL;
+#endif
+}
+
+
+PowerManagement::~PowerManagement() {
+  allowSystemSleep(true);
+  allowDisplaySleep(true);
+#ifdef HAVE_SYSTEMD
+  pri->bus = sd_bus_flush_close_unref(pri->bus);
+#endif
+  zap(pri);
 }

@@ -218,37 +218,25 @@ void PowerManagement::updateIdleSeconds() {
   }

 #else
-  // NOTE We use dynamic library access to avoid a direct dependency on X11
-  try {
-    if (!pri->initialized) {
-      pri->initialized = true;
-
-      // Get display
-      DynamicLibrary xLib("libX11.so");
-      pri->display = xLib.accessSymbol<XOpenDisplay_t>("XOpenDisplay")(0);
-      if (!pri->display) return;
-
-      // Get default root window
-      pri->root = xLib.accessSymbol<XDefaultRootWindow_t>
-        ("XDefaultRootWindow")(pri->display);
-
-      // Get XScreensaver lib
-      pri->xssLib = new DynamicLibrary("libXss.so");
-
-      // Allocate XScreenSaverInfo
-      pri->info = pri->xssLib->accessSymbol<XScreenSaverAllocInfo_t>
-        ("XScreenSaverAllocInfo")();
-    }
-
-    if (!pri->display || !pri->info) return;
-
-    // Query idle state
-    pri->xssLib->accessSymbol<XScreenSaverQueryInfo_t>
-      ("XScreenSaverQueryInfo")(pri->display, pri->root, pri->info);
-
-    idleSeconds = pri->info->idle / 1000; // Convert from ms.
-
-  } catch (...) {} // Ignore
+#ifdef HAVE_SYSTEMD
+  int r, idle;
+
+  if (!pri->bus) return;
+
+  r = sd_bus_get_property_trivial(pri->bus,
+                                  "org.freedesktop.login1",
+                                  "/org/freedesktop/login1/seat/seat0",
+                                  "org.freedesktop.login1.Seat",
+                                  "IdleHint",
+                                  NULL,
+                                  'b',
+                                  &idle);
+
+  if (r >= 0 && idle != 0)
+    // logind API does not exactly match Windows and macOS ones,
+    // if IdleHint is true, assume enough time without input has passed
+    idleSeconds = -1;
+#endif // HAVE_SYSTEMD
 #endif
 }

diff --git a/src/cbang/os/PowerManagement.h b/src/cbang/os/PowerManagement.h
index f535f32a..cb2e77d5 100644
--- a/src/cbang/os/PowerManagement.h
+++ b/src/cbang/os/PowerManagement.h
@@ -31,13 +31,15 @@

 #pragma once

-#include <cbang/util/Singleton.h>
+#include <cbang/config.h>
+#include <cstdint>

 namespace cb {
   class Info;

-  class PowerManagement : public Singleton<PowerManagement> {
+  class PowerManagement {
+  protected:
     uint64_t lastBatteryUpdate;
     bool systemOnBattery;
     bool systemHasBattery;
@@ -52,7 +54,8 @@ namespace cb {
     private_t *pri;

   public:
-    PowerManagement(Inaccessible);
+    PowerManagement();
+    ~PowerManagement();

     bool onBattery();
     bool hasBattery();
diff --git a/src/cbang/os/SystemInfo.cpp b/src/cbang/os/SystemInfo.cpp
index d2557851..83730e5e 100644
--- a/src/cbang/os/SystemInfo.cpp
+++ b/src/cbang/os/SystemInfo.cpp
@@ -308,9 +308,9 @@ void SystemInfo::add(Info &info) {
   info.add(category, "OS Version", osVersion.toString());

   info.add(category, "Has Battery",
-           String(PowerManagement::instance().hasBattery()));
+           String(PowerManagement().hasBattery()));
   info.add(category, "On Battery",
-           String(PowerManagement::instance().onBattery()));
+           String(PowerManagement().onBattery()));

   try {
     info.add(category, "Hostname", getHostname());

Client:

diff --git a/src/fah/client/lin/LinOSImpl.cpp b/src/fah/client/lin/LinOSImpl.cpp
index f97ca63..5479c18 100644
--- a/src/fah/client/lin/LinOSImpl.cpp
+++ b/src/fah/client/lin/LinOSImpl.cpp
@@ -37,6 +37,9 @@ using namespace std;
 using namespace cb;

+namespace {PowerManagement pm;}
+
+
 const char *LinOSImpl::getName() const {return "linux";}

@@ -54,5 +57,5 @@ const char *LinOSImpl::getCPU() const {

 bool LinOSImpl::isSystemIdle() const {
-  return 15 < PowerManagement::instance().getIdleSeconds();
+  return 15 < pm.getIdleSeconds();
 }

Can you C++ guys take a look (from macOS and Windows POV too)?

[jcoffland]

Looks reasonable to me.

[marcosfrm]

@jcoffland Please see https://github.com/CauldronDevelopmentLLC/cbang/pull/129#issuecomment-1827643170. Does it look ok? The destructor will work with it and there will be no need to call shutdown() on client side.

[marcosfrm]

@jcoffland diff from master:

diff --git a/src/cbang/os/PowerManagement.cpp b/src/cbang/os/PowerManagement.cpp
index 236b8cf9..984ad758 100644
--- a/src/cbang/os/PowerManagement.cpp
+++ b/src/cbang/os/PowerManagement.cpp
@@ -36,6 +36,7 @@
 #include <cbang/os/SystemUtilities.h>
 #include <cbang/Catch.h>
 #include <cbang/time/Time.h>
+#include <cbang/Zap.h>

 #if defined(_WIN32)
 #define WIN32_LEAN_AND_MEAN // Avoid including winsock.h
@@ -69,7 +70,7 @@ struct PowerManagement::private_t {
 };

-PowerManagement::PowerManagement(Inaccessible) :
+PowerManagement::PowerManagement() :
   lastBatteryUpdate(0), systemOnBattery(false), systemHasBattery(false),
   lastIdleSecondsUpdate(0), idleSeconds(0), systemSleepAllowed(true),
   displaySleepAllowed(true), pri(new private_t) {
@@ -81,13 +82,14 @@ PowerManagement::PowerManagement(Inaccessible) :
 }

-void PowerManagement::shutdown() {
+PowerManagement::~PowerManagement() {
   allowSystemSleep(true);
   allowDisplaySleep(true);

 #if defined(HAVE_SYSTEMD)
   pri->bus = sd_bus_flush_close_unref(pri->bus);
 #endif
+  zap(pri);
 }

diff --git a/src/cbang/os/PowerManagement.h b/src/cbang/os/PowerManagement.h
index 886984e3..4058def8 100644
--- a/src/cbang/os/PowerManagement.h
+++ b/src/cbang/os/PowerManagement.h
@@ -31,13 +31,14 @@

 #pragma once

-#include <cbang/util/Singleton.h>
+#include <cstdint>

 namespace cb {
   class Info;

-  class PowerManagement : public Singleton<PowerManagement> {
+  class PowerManagement {
+  protected:
     uint64_t lastBatteryUpdate;
     bool systemOnBattery;
     bool systemHasBattery;
@@ -52,7 +53,8 @@ namespace cb {
     private_t *pri;

   public:
-    PowerManagement(Inaccessible);
+    PowerManagement();
+    ~PowerManagement();

     bool onBattery();
     bool hasBattery();
@@ -60,8 +62,6 @@ namespace cb {
     void allowSystemSleep(bool x);
     void allowDisplaySleep(bool x);

-    void shutdown();
-
   protected:
     void updateIdleSeconds();
     void updateBatteryInfo();
diff --git a/src/cbang/os/SystemInfo.cpp b/src/cbang/os/SystemInfo.cpp
index d2557851..83730e5e 100644
--- a/src/cbang/os/SystemInfo.cpp
+++ b/src/cbang/os/SystemInfo.cpp
@@ -308,9 +308,9 @@ void SystemInfo::add(Info &info) {
   info.add(category, "OS Version", osVersion.toString());

   info.add(category, "Has Battery",
-           String(PowerManagement::instance().hasBattery()));
+           String(PowerManagement().hasBattery()));
   info.add(category, "On Battery",
-           String(PowerManagement::instance().onBattery()));
+           String(PowerManagement().onBattery()));

   try {
     info.add(category, "Hostname", getHostname());

[jcoffland]

We don't need to make PowerManagement not a singleton. Simply moving PowerManagement::shutdown() to PowerManagement::~PowerManagement() is sufficient. It will be automatically destructed on program exit. I'll make this change.