[libvirt] [PATCH v4] Introduce --without-pm-utils to get rid of pm-is-supported dependency
Jim Fehlig
jfehlig at suse.com
Fri Apr 11 04:31:47 UTC 2014
Cedric Bosdonnat wrote:
> Could anyone have a look at this one?
>
Sure :).
> --
> Cedric
>
> On Thu, 2014-04-03 at 14:20 +0200, Cédric Bosdonnat wrote:
>
>> This uses the dbus api of systemd to check the power management
>> capabilities of the node.
>> ---
>> Diff with v3:
>> * Added unit tests vir virSystemdCan* helpers
>> * Make the default for with-pm-utils depending on dbus and systemd detection
>> * Changed the implementation of the helpers to return true for
>> 'yes' and 'challenge' responses, we shouldn't get a 'no' given libvirtd runs
>> as privileged user... but who knows.
>>
>> configure.ac | 24 +++++++++++
>> 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/virsystemdmock.c | 22 ++++++++++
>>
Needs rebased after 4607168e, which removed this file.
>> tests/virsystemdtest.c | 100 +++++++++++++++++++++++++++++++++++++++++++++-
>> 8 files changed, 281 insertions(+), 17 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 73efffa..34e5ec2 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,25 @@ fi
>>
>> AM_CONDITIONAL([WITH_PHYP],[test "$with_phyp" = "yes"])
>>
>> +dnl
>> +dnl Should we build with pm-utils support?
>> +dnl
>> +set -x
>>
Left-over debug?
>> +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"])
>> +set +x
>>
Same.
>> +
>> dnl virsh libraries
>> VIRSH_LIBS="$VIRSH_LIBS $READLINE_LIBS"
>> AC_SUBST([VIRSH_LIBS])
>> @@ -2845,6 +2868,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 eab9b23..5c20955 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}
>> %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 38fbf63..ce51bdf 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1879,6 +1879,9 @@ virSysinfoSetup;
>>
>>
>> # util/virsystemd.h
>> +virSystemdCanHibernate;
>> +virSystemdCanHybridSleep;
>> +virSystemdCanSuspend;
>> virSystemdCreateMachine;
>> virSystemdMakeMachineName;
>> virSystemdMakeScopeName;
>> diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c
>> index 8088931..b1eddff 100644
>> --- a/src/util/virnodesuspend.c
>> +++ b/src/util/virnodesuspend.c
>> @@ -22,6 +22,7 @@
>> #include <config.h>
>> #include "virnodesuspend.h"
>>
>> +# include "virsystemd.h"
>> #include "vircommand.h"
>> #include "virthread.h"
>> #include "datatypes.h"
>> @@ -245,23 +246,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;
>> @@ -300,6 +287,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;
>> +
>> + *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/virsystemdmock.c b/tests/virsystemdmock.c
>> index 23167db..cc385ec 100644
>> --- a/tests/virsystemdmock.c
>> +++ b/tests/virsystemdmock.c
>> @@ -76,10 +76,21 @@ DBusMessage *dbus_connection_send_with_reply_and_block(DBusConnection *connectio
>> } 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, sub;
>> reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN);
>> dbus_message_iter_init_append(reply, &iter);
>> @@ -95,11 +106,17 @@ DBusMessage *dbus_connection_send_with_reply_and_block(DBusConnection *connectio
>> 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, sub;
>> reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN);
>> dbus_message_iter_init_append(reply, &iter);
>> @@ -115,6 +132,11 @@ DBusMessage *dbus_connection_send_with_reply_and_block(DBusConnection *connectio
>> 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);
>> diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c
>> index 4fc5137..a9583e7 100644
>> --- a/tests/virsystemdtest.c
>> +++ b/tests/virsystemdtest.c
>> @@ -205,7 +205,6 @@ static int testCreateBadSystemd(const void *opaque ATTRIBUTE_UNUSED)
>> return 0;
>> }
>>
>> -
>>
Spurious whitespace change.
Looks good otherwise. You addressed all the comments from previous
versions.
Regards,
Jim
>> struct testScopeData {
>> const char *name;
>> const char *partition;
>> @@ -237,6 +236,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:
>> + 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)
>> {
>> @@ -275,6 +354,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;
>>
>> }
>>
>>
>
>
More information about the libvir-list
mailing list