Cédric Bosdonnat wrote:

This uses the dbus api of systemd to check the power management capabilities of the node. --- Difference with v4: * Removed left-over debug bits * Rebased * Cleaned up the spurious white space change

configure.ac | 22 +++++++++ libvirt.spec.in | 9 ++++ src/libvirt_private.syms | 3 ++ src/util/virnodesuspend.c | 75 ++++++++++++++++++++++------ src/util/virsystemd.c | 59 ++++++++++++++++++++++ src/util/virsystemd.h | 6 +++ tests/virsystemdtest.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 279 insertions(+), 16 deletions(-)

diff --git a/configure.ac b/configure.ac index 52c50df..ea85851 100644 --- a/configure.ac +++ b/configure.ac @@ -563,6 +563,10 @@ AC_ARG_WITH([chrdev-lock-files], [location for UUCP style lock files for character devices (use auto for default paths on some platforms) @<:@default=auto@:>@])]) m4_divert_text([DEFAULTS], [with_chrdev_lock_files=auto]) +AC_ARG_WITH([pm-utils], + [AS_HELP_STRING([--with-pm-utils], + [use pm-utils for power management @<:@default=yes@:>@])]) +m4_divert_text([DEFAULTS], [with_pm_utils=check])

dnl dnl in case someone want to build static binaries @@ -1621,6 +1625,23 @@ fi

AM_CONDITIONAL([WITH_PHYP],[test "$with_phyp" = "yes"])

+dnl +dnl Should we build with pm-utils support? +dnl +if test "$with_pm_utils" = "check"; then + with_pm_utils=yes + if test "$with_dbus" = "yes"; then + if test "$init_systemd" = "yes"; then + with_pm_utils=no + fi + fi +fi + +if test "$with_pm_utils" = "yes"; then + AC_DEFINE_UNQUOTED([WITH_PM_UTILS], 1, [whether to use pm-utils]) +fi +AM_CONDITIONAL([WITH_PM_UTILS], [test "$with_pm_utils" = "yes"]) + dnl virsh libraries VIRSH_LIBS="$VIRSH_LIBS $READLINE_LIBS" AC_SUBST([VIRSH_LIBS]) @@ -2853,6 +2874,7 @@ AC_MSG_NOTICE([ rbd: $LIBRBD_LIBS]) else AC_MSG_NOTICE([ rbd: no]) fi +AC_MSG_NOTICE([pm-utils: $with_pm_utils])

AC_MSG_NOTICE([]) AC_MSG_NOTICE([Test suite]) diff --git a/libvirt.spec.in b/libvirt.spec.in index 8e83fb3..b61ef8d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -132,6 +132,7 @@ %define with_libssh2 0%{!?_without_libssh2:0} %define with_wireshark 0%{!?_without_wireshark:0} %define with_systemd_daemon 0%{!?_without_systemd_daemon:0} +%define with_pm_utils 1

# Non-server/HV driver defaults which are always enabled %define with_sasl 0%{!?_without_sasl:1} @@ -182,6 +183,7 @@ %if 0%{?fedora} >= 17 || 0%{?rhel} >= 7 %define with_systemd 1 %define with_systemd_daemon 1 + %define with_pm_utils 0 %endif

# Fedora 18 / RHEL-7 are first where firewalld support is enabled @@ -1138,8 +1140,10 @@ Requires: nc Requires: gettext # Needed by virt-pki-validate script. Requires: gnutls-utils +%if %{with_pm_utils} # Needed for probing the power management features of the host. Requires: pm-utils +%{endif}

%endif

%if %{with_sasl} Requires: cyrus-sasl # Not technically required, but makes 'out-of-box' config @@ -1395,6 +1399,10 @@ driver %define _without_systemd_daemon --without-systemd-daemon %endif

+%if ! %{with_pm_utils} + %define _without_pm_utils --without-pm-utils +%endif + %define when %(date +"%%F-%%T") %define where %(hostname) %define who %{?packager}%{!?packager:Unknown} @@ -1471,6 +1479,7 @@ rm -f po/stamp-po %{?_with_firewalld} \ %{?_without_wireshark} \ %{?_without_systemd_daemon} \ + %{?_without_pm_utils} \ %{with_packager} \ %{with_packager_version} \ --with-qemu-user=%{qemu_user} \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cd43335..6e807c4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1888,6 +1888,9 @@ virSysinfoSetup;

# util/virsystemd.h +virSystemdCanHibernate; +virSystemdCanHybridSleep; +virSystemdCanSuspend; virSystemdCreateMachine; virSystemdMakeMachineName; virSystemdMakeScopeName; diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 08c47aa..70ede2a 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -22,6 +22,7 @@ #include <config.h> #include "virnodesuspend.h"

+# include "virsystemd.h"

Fails make syntax-check with cppi installed.

#include "vircommand.h" #include "virthread.h" #include "datatypes.h" @@ -228,23 +229,9 @@ int nodeSuspendForDuration(unsigned int target, return ret; }

- -/** - * virNodeSuspendSupportsTarget: - * @target: The power management target to check whether it is supported - * by the host. Values could be: - * VIR_NODE_SUSPEND_TARGET_MEM - * VIR_NODE_SUSPEND_TARGET_DISK - * VIR_NODE_SUSPEND_TARGET_HYBRID - * @supported: set to true if supported, false otherwise - * - * Run the script 'pm-is-supported' (from the pm-utils package) - * to find out if @target is supported by the host. - * - * Returns 0 if the query was successful, -1 on failure. - */ +#ifdef WITH_PM_UTILS static int -virNodeSuspendSupportsTarget(unsigned int target, bool *supported) +virNodeSuspendSupportsTargetPMUtils(unsigned int target, bool *supported) { virCommandPtr cmd; int status; @@ -280,6 +267,62 @@ virNodeSuspendSupportsTarget(unsigned int target, bool *supported) virCommandFree(cmd); return ret; } +#endif + +static int +virNodeSuspendSupportsTargetSystemd(unsigned int target, bool *supported) +{ + int ret = -1; + + if (virNodeSuspendInitialize() < 0) + return -1;

This can be removed after 3f671e6c.

+ + *supported = false; + + switch (target) { + case VIR_NODE_SUSPEND_TARGET_MEM: + ret = virSystemdCanSuspend(supported); + break; + case VIR_NODE_SUSPEND_TARGET_DISK: + ret = virSystemdCanHibernate(supported); + break; + case VIR_NODE_SUSPEND_TARGET_HYBRID: + ret = virSystemdCanHybridSleep(supported); + break; + default: + return ret; + } + + return ret; +} + +/** + * virNodeSuspendSupportsTarget: + * @target: The power management target to check whether it is supported + * by the host. Values could be: + * VIR_NODE_SUSPEND_TARGET_MEM + * VIR_NODE_SUSPEND_TARGET_DISK + * VIR_NODE_SUSPEND_TARGET_HYBRID + * @supported: set to true if supported, false otherwise + * + * Run the script 'pm-is-supported' (from the pm-utils package) + * to find out if @target is supported by the host. + * + * Returns 0 if the query was successful, -1 on failure. + */ +static int +virNodeSuspendSupportsTarget(unsigned int target, bool *supported) +{ + int ret; + + ret = virNodeSuspendSupportsTargetSystemd(target, supported); +#ifdef WITH_PM_UTILS + if (ret < 0) + ret = virNodeSuspendSupportsTargetPMUtils(target, supported); +#endif + + return ret; +}

/** * virNodeSuspendGetTargetMask: diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 93b3f9c..e9ca564 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -325,3 +325,62 @@ virSystemdNotifyStartup(void) sd_notify(0, "READY=1"); #endif } + +static int +virSystemdPMSupportTarget(const char *methodName, bool *result) +{ + int ret; + DBusConnection *conn; + DBusMessage *message = NULL; + char *response; + + ret = virDBusIsServiceEnabled("org.freedesktop.login1"); + if (ret < 0) + return ret; + + if ((ret = virDBusIsServiceRegistered("org.freedesktop.login1")) < 0) + return ret; + + if (!(conn = virDBusGetSystemBus())) + return -1; + + ret = -1; + + if (virDBusCallMethod(conn, + &message, + NULL, + "org.freedesktop.login1", + "/org/freedesktop/login1", + "org.freedesktop.login1.Manager", + methodName, + NULL) < 0) + return ret; + + if ((ret = virDBusMessageRead(message, "s", &response)) < 0) + goto cleanup; + + *result = STREQ("yes", response) || STREQ("challenge", response); + + ret = 0; + + cleanup: + dbus_message_unref(message); + VIR_FREE(response); + + return ret; +} + +int virSystemdCanSuspend(bool *result) +{ + return virSystemdPMSupportTarget("CanSuspend", result); +} + +int virSystemdCanHibernate(bool *result) +{ + return virSystemdPMSupportTarget("CanHibernate", result); +} + +int virSystemdCanHybridSleep(bool *result) +{ + return virSystemdPMSupportTarget("CanHybridSleep", result); +} diff --git a/src/util/virsystemd.h b/src/util/virsystemd.h index 7fed456..491c9b7 100644 --- a/src/util/virsystemd.h +++ b/src/util/virsystemd.h @@ -48,4 +48,10 @@ int virSystemdTerminateMachine(const char *name,

void virSystemdNotifyStartup(void);

+int virSystemdCanSuspend(bool *result); + +int virSystemdCanHibernate(bool *result); + +int virSystemdCanHybridSleep(bool *result); + #endif /* __VIR_SYSTEMD_H__ */ diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 2b014cc..86d717f 100644 --- a/tests/virsystemdtest.c +++ b/tests/virsystemdtest.c @@ -55,10 +55,21 @@ VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block, } else { reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); } + } else if (STREQ(service, "org.freedesktop.login1")) { + char *supported = getenv("RESULT_SUPPORT"); + DBusMessageIter iter; + reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); + dbus_message_iter_init_append(reply, &iter); + + if (!dbus_message_iter_append_basic(&iter, + DBUS_TYPE_STRING, + &supported)) + goto error; } else if (STREQ(service, "org.freedesktop.DBus") && STREQ(member, "ListActivatableNames")) { const char *svc1 = "org.foo.bar.wizz"; const char *svc2 = "org.freedesktop.machine1"; + const char *svc3 = "org.freedesktop.login1"; DBusMessageIter iter; DBusMessageIter sub; reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); @@ -75,11 +86,17 @@ VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block, DBUS_TYPE_STRING, &svc2)) goto error; + if (!getenv("FAIL_NO_SERVICE") && + !dbus_message_iter_append_basic(&sub, + DBUS_TYPE_STRING, + &svc3)) + goto error; dbus_message_iter_close_container(&iter, &sub); } else if (STREQ(service, "org.freedesktop.DBus") && STREQ(member, "ListNames")) { const char *svc1 = "org.foo.bar.wizz"; const char *svc2 = "org.freedesktop.systemd1"; + const char *svc3 = "org.freedesktop.login1"; DBusMessageIter iter; DBusMessageIter sub; reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); @@ -96,6 +113,11 @@ VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block, DBUS_TYPE_STRING, &svc2)) goto error; + if ((!getenv("FAIL_NO_SERVICE") && !getenv("FAIL_NOT_REGISTERED")) && + !dbus_message_iter_append_basic(&sub, + DBUS_TYPE_STRING, + &svc3)) + goto error; dbus_message_iter_close_container(&iter, &sub); } else { reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); @@ -313,6 +335,86 @@ testScopeName(const void *opaque) return ret; }

+typedef int (*virSystemdCanHelper)(bool * result); +struct testPMSupportData { + virSystemdCanHelper tested; +}; + +static int testPMSupportHelper(const void *opaque) +{ + int rv; + bool result; + size_t i; + const char *results[4] = {"yes", "no", "na", "challenge"}; + int expected[4] = {1, 0, 0, 1}; + const struct testPMSupportData *data = opaque; + + for (i = 0; i < 4; i++) { + setenv("RESULT_SUPPORT", results[i], 1); + if ((rv = data->tested(&result)) < 0) { + fprintf(stderr, "%s", "Unexpected canSuspend error\n"); + return -1; + } + + if (result != expected[i]) { + fprintf(stderr, "Unexpected result for answer '%s'\n", results[i]); + goto error; + } + unsetenv("RESULT_SUPPORT"); + } + + return 0; +error:

Whitespace off on the label.

+ unsetenv("RESULT_SUPPORT"); + return -1; +} + +static int testPMSupportHelperNoSystemd(const void *opaque) +{ + int rv; + bool result; + const struct testPMSupportData *data = opaque; + + setenv("FAIL_NO_SERVICE", "1", 1); + + if ((rv = data->tested(&result)) == 0) { + unsetenv("FAIL_NO_SERVICE"); + fprintf(stderr, "%s", "Unexpected canSuspend success\n"); + return -1; + } + unsetenv("FAIL_NO_SERVICE"); + + if (rv != -2) { + fprintf(stderr, "%s", "Unexpected canSuspend error\n"); + return -1; + } + + return 0; +} + +static int testPMSupportSystemdNotRunning(const void *opaque) +{ + int rv; + bool result; + const struct testPMSupportData *data = opaque; + + setenv("FAIL_NOT_REGISTERED", "1", 1); + + if ((rv = data->tested(&result)) == 0) { + unsetenv("FAIL_NOT_REGISTERED"); + fprintf(stderr, "%s", "Unexpected canSuspend success\n"); + return -1; + } + unsetenv("FAIL_NOT_REGISTERED"); + + if (rv != -2) { + fprintf(stderr, "%s", "Unexpected canSuspend error\n"); + return -1; + } + + return 0; +} + static int mymain(void) { @@ -351,6 +453,25 @@ mymain(void) TEST_SCOPE("demo", "/machine/eng-dept/testing!stuff", "machine-eng\\x2ddept-testing\\x21stuff-lxc\\x2ddemo.scope");

+# define TESTS_PM_SUPPORT_HELPER(name, function) \ + do { \ + struct testPMSupportData data = { \ + function \ + }; \ + if (virtTestRun("Test " name " ", testPMSupportHelper, &data) < 0) \ + ret = -1; \ + if (virtTestRun("Test " name " no systemd ", \ + testPMSupportHelperNoSystemd, &data) < 0) \ + ret = -1; \ + if (virtTestRun("Test systemd " name " not running ", \ + testPMSupportSystemdNotRunning, &data) < 0) \ + ret = -1; \ + } while (0) + + TESTS_PM_SUPPORT_HELPER("canSuspend", &virSystemdCanSuspend); + TESTS_PM_SUPPORT_HELPER("canHibernate", &virSystemdCanHibernate); + TESTS_PM_SUPPORT_HELPER("canHybridSleep", &virSystemdCanHybridSleep); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; }

But looks good otherwise. I've tested this on a systemd machine and an older sysvinit system that uses pm-is-supported. I'll squash in the below diff and push shortly. Regards, Jim diff --git a/libvirt.spec.in b/libvirt.spec.in index b61ef8d..4e70a41 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1143,7 +1143,7 @@ Requires: gnutls-utils %if %{with_pm_utils} # Needed for probing the power management features of the host. Requires: pm-utils -%{endif} +%endif %if %{with_sasl} Requires: cyrus-sasl # Not technically required, but makes 'out-of-box' config diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 70ede2a..59b84ef 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -22,7 +22,7 @@ #include <config.h> #include "virnodesuspend.h" -# include "virsystemd.h" +#include "virsystemd.h" #include "vircommand.h" #include "virthread.h" #include "datatypes.h" @@ -274,9 +274,6 @@ virNodeSuspendSupportsTargetSystemd(unsigned int target, bool *supported) { int ret = -1; - if (virNodeSuspendInitialize() < 0) - return -1; - *supported = false; switch (target) { diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 86d717f..0fcd4e8 100644 --- a/tests/virsystemdtest.c +++ b/tests/virsystemdtest.c @@ -364,7 +364,7 @@ static int testPMSupportHelper(const void *opaque) } return 0; -error: + error: unsetenv("RESULT_SUPPORT"); return -1; }