* [Qemu-devel] [PATCH v1 1/6] qga: refactoring qmp_guest_suspend_* functions
2018-06-19 19:38 [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep Daniel Henrique Barboza
@ 2018-06-19 19:38 ` Daniel Henrique Barboza
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 2/6] qga: bios_supports_mode: decoupling pm-utils and sys logic Daniel Henrique Barboza
` (5 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Daniel Henrique Barboza @ 2018-06-19 19:38 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, mdroth, Daniel Henrique Barboza
To be able to add new suspend mechanisms we need to detach
the existing QMP functions from the current implementation
specifics.
At this moment we have functions such as qmp_guest_suspend_ram
calling bios_suspend_mode and guest_suspend passing the
pmutils command and arguments as parameters. This patch
removes this logic from the QMP functions, moving them to
the respective functions that will have to deal with which
binary to use.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
qga/commands-posix.c | 87 ++++++++++++++++++++++++++++----------------
1 file changed, 55 insertions(+), 32 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index eae817191b..63c49791a4 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1438,15 +1438,38 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
#define LINUX_SYS_STATE_FILE "/sys/power/state"
#define SUSPEND_SUPPORTED 0
#define SUSPEND_NOT_SUPPORTED 1
+#define SUSPEND_MODE_DISK 1
+#define SUSPEND_MODE_RAM 2
+#define SUSPEND_MODE_HYBRID 3
-static void bios_supports_mode(const char *pmutils_bin, const char *pmutils_arg,
- const char *sysfile_str, Error **errp)
+static void bios_supports_mode(int suspend_mode, Error **errp)
{
Error *local_err = NULL;
+ const char *pmutils_arg, *sysfile_str;
+ const char *pmutils_bin = "pm-is-supported";
char *pmutils_path;
pid_t pid;
int status;
+ switch (suspend_mode) {
+
+ case SUSPEND_MODE_DISK:
+ pmutils_arg = "--hibernate";
+ sysfile_str = "disk";
+ break;
+ case SUSPEND_MODE_RAM:
+ pmutils_arg = "--suspend";
+ sysfile_str = "mem";
+ break;
+ case SUSPEND_MODE_HYBRID:
+ pmutils_arg = "--suspend-hybrid";
+ sysfile_str = NULL;
+ break;
+ default:
+ error_setg(errp, "guest suspend mode not supported");
+ return;
+ }
+
pmutils_path = g_find_program_in_path(pmutils_bin);
pid = fork();
@@ -1523,14 +1546,39 @@ out:
g_free(pmutils_path);
}
-static void guest_suspend(const char *pmutils_bin, const char *sysfile_str,
- Error **errp)
+static void guest_suspend(int suspend_mode, Error **errp)
{
Error *local_err = NULL;
+ const char *pmutils_bin, *sysfile_str;
char *pmutils_path;
pid_t pid;
int status;
+ bios_supports_mode(suspend_mode, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ switch (suspend_mode) {
+
+ case SUSPEND_MODE_DISK:
+ pmutils_bin = "pm-hibernate";
+ sysfile_str = "disk";
+ break;
+ case SUSPEND_MODE_RAM:
+ pmutils_bin = "pm-suspend";
+ sysfile_str = "mem";
+ break;
+ case SUSPEND_MODE_HYBRID:
+ pmutils_bin = "pm-suspend-hybrid";
+ sysfile_str = NULL;
+ break;
+ default:
+ error_setg(errp, "unknown guest suspend mode");
+ return;
+ }
+
pmutils_path = g_find_program_in_path(pmutils_bin);
pid = fork();
@@ -1593,42 +1641,17 @@ out:
void qmp_guest_suspend_disk(Error **errp)
{
- Error *local_err = NULL;
-
- bios_supports_mode("pm-is-supported", "--hibernate", "disk", &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
-
- guest_suspend("pm-hibernate", "disk", errp);
+ guest_suspend(SUSPEND_MODE_DISK, errp);
}
void qmp_guest_suspend_ram(Error **errp)
{
- Error *local_err = NULL;
-
- bios_supports_mode("pm-is-supported", "--suspend", "mem", &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
-
- guest_suspend("pm-suspend", "mem", errp);
+ guest_suspend(SUSPEND_MODE_RAM, errp);
}
void qmp_guest_suspend_hybrid(Error **errp)
{
- Error *local_err = NULL;
-
- bios_supports_mode("pm-is-supported", "--suspend-hybrid", NULL,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
-
- guest_suspend("pm-suspend-hybrid", NULL, errp);
+ guest_suspend(SUSPEND_MODE_HYBRID, errp);
}
static GuestNetworkInterfaceList *
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [Qemu-devel] [PATCH v1 2/6] qga: bios_supports_mode: decoupling pm-utils and sys logic
2018-06-19 19:38 [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep Daniel Henrique Barboza
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 1/6] qga: refactoring qmp_guest_suspend_* functions Daniel Henrique Barboza
@ 2018-06-19 19:38 ` Daniel Henrique Barboza
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 3/6] qga: guest_suspend: " Daniel Henrique Barboza
` (4 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Daniel Henrique Barboza @ 2018-06-19 19:38 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, mdroth, Daniel Henrique Barboza
In bios_supports_mode there is a verification to assert if
the chosen suspend mode is supported by the pmutils tools and,
if not, we see if the Linux sys state files supports it.
This verification is done in the same function, one after
the other, and it works for now. But, when adding a new
suspend mechanism that will not necessarily follow the same
return 0 or 1 logic of pmutils, this code will be hard
to deal with.
This patch decouple the two existing logics into their own
functions, pmutils_supports_mode and linux_sys_state_supports_mode,
which in turn are used inside bios_support_mode. The existing
logic is kept but now it's easier to extend it.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
qga/commands-posix.c | 116 +++++++++++++++++++++++++------------------
1 file changed, 68 insertions(+), 48 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 63c49791a4..89ffd8dc88 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1442,75 +1442,43 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
#define SUSPEND_MODE_RAM 2
#define SUSPEND_MODE_HYBRID 3
-static void bios_supports_mode(int suspend_mode, Error **errp)
+static bool pmutils_supports_mode(int suspend_mode, Error **errp)
{
Error *local_err = NULL;
- const char *pmutils_arg, *sysfile_str;
+ const char *pmutils_arg;
const char *pmutils_bin = "pm-is-supported";
char *pmutils_path;
pid_t pid;
int status;
+ bool ret = false;
switch (suspend_mode) {
case SUSPEND_MODE_DISK:
pmutils_arg = "--hibernate";
- sysfile_str = "disk";
break;
case SUSPEND_MODE_RAM:
pmutils_arg = "--suspend";
- sysfile_str = "mem";
break;
case SUSPEND_MODE_HYBRID:
pmutils_arg = "--suspend-hybrid";
- sysfile_str = NULL;
break;
default:
- error_setg(errp, "guest suspend mode not supported");
- return;
+ return ret;
}
pmutils_path = g_find_program_in_path(pmutils_bin);
+ if (!pmutils_path) {
+ return ret;
+ }
pid = fork();
if (!pid) {
- char buf[32]; /* hopefully big enough */
- ssize_t ret;
- int fd;
-
setsid();
- reopen_fd_to_null(0);
- reopen_fd_to_null(1);
- reopen_fd_to_null(2);
-
- if (pmutils_path) {
- execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
- }
-
+ execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
/*
- * If we get here either pm-utils is not installed or execle() has
- * failed. Let's try the manual method if the caller wants it.
+ * If we get here execle() has failed.
*/
-
- if (!sysfile_str) {
- _exit(SUSPEND_NOT_SUPPORTED);
- }
-
- fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
- if (fd < 0) {
- _exit(SUSPEND_NOT_SUPPORTED);
- }
-
- ret = read(fd, buf, sizeof(buf)-1);
- if (ret <= 0) {
- _exit(SUSPEND_NOT_SUPPORTED);
- }
- buf[ret] = '\0';
-
- if (strstr(buf, sysfile_str)) {
- _exit(SUSPEND_SUPPORTED);
- }
-
_exit(SUSPEND_NOT_SUPPORTED);
} else if (pid < 0) {
error_setg_errno(errp, errno, "failed to create child process");
@@ -1523,17 +1491,11 @@ static void bios_supports_mode(int suspend_mode, Error **errp)
goto out;
}
- if (!WIFEXITED(status)) {
- error_setg(errp, "child process has terminated abnormally");
- goto out;
- }
-
switch (WEXITSTATUS(status)) {
case SUSPEND_SUPPORTED:
+ ret = true;
goto out;
case SUSPEND_NOT_SUPPORTED:
- error_setg(errp,
- "the requested suspend mode is not supported by the guest");
goto out;
default:
error_setg(errp,
@@ -1544,6 +1506,64 @@ static void bios_supports_mode(int suspend_mode, Error **errp)
out:
g_free(pmutils_path);
+ return ret;
+}
+
+static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
+{
+ const char *sysfile_str;
+ char buf[32]; /* hopefully big enough */
+ int fd;
+ ssize_t ret;
+
+ switch (suspend_mode) {
+
+ case SUSPEND_MODE_DISK:
+ sysfile_str = "disk";
+ break;
+ case SUSPEND_MODE_RAM:
+ sysfile_str = "mem";
+ break;
+ default:
+ return false;
+ }
+
+ fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
+ if (fd < 0) {
+ return false;
+ }
+
+ ret = read(fd, buf, sizeof(buf) - 1);
+ if (ret <= 0) {
+ return false;
+ }
+ buf[ret] = '\0';
+
+ if (strstr(buf, sysfile_str)) {
+ return true;
+ }
+ return false;
+}
+
+static void bios_supports_mode(int suspend_mode, Error **errp)
+{
+ Error *local_err = NULL;
+ bool ret;
+
+ ret = pmutils_supports_mode(suspend_mode, &local_err);
+ if (ret) {
+ return;
+ }
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ ret = linux_sys_state_supports_mode(suspend_mode, errp);
+ if (!ret) {
+ error_setg(errp,
+ "the requested suspend mode is not supported by the guest");
+ return;
+ }
}
static void guest_suspend(int suspend_mode, Error **errp)
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [Qemu-devel] [PATCH v1 3/6] qga: guest_suspend: decoupling pm-utils and sys logic
2018-06-19 19:38 [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep Daniel Henrique Barboza
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 1/6] qga: refactoring qmp_guest_suspend_* functions Daniel Henrique Barboza
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 2/6] qga: bios_supports_mode: decoupling pm-utils and sys logic Daniel Henrique Barboza
@ 2018-06-19 19:38 ` Daniel Henrique Barboza
2018-06-19 23:23 ` Marc-André Lureau
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 4/6] qga: removing switch statements, adding run_process_child Daniel Henrique Barboza
` (3 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: Daniel Henrique Barboza @ 2018-06-19 19:38 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, mdroth, Daniel Henrique Barboza
Following the same logic of the previous patch, let's also
decouple the suspend logic from guest_suspend into specialized
functions, one for each strategy we support at this moment.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
qga/commands-posix.c | 170 +++++++++++++++++++++++++++----------------
1 file changed, 108 insertions(+), 62 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 89ffd8dc88..a2870f9ab9 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1509,6 +1509,65 @@ out:
return ret;
}
+static void pmutils_suspend(int suspend_mode, Error **errp)
+{
+ Error *local_err = NULL;
+ const char *pmutils_bin;
+ char *pmutils_path;
+ pid_t pid;
+ int status;
+
+ switch (suspend_mode) {
+
+ case SUSPEND_MODE_DISK:
+ pmutils_bin = "pm-hibernate";
+ break;
+ case SUSPEND_MODE_RAM:
+ pmutils_bin = "pm-suspend";
+ break;
+ case SUSPEND_MODE_HYBRID:
+ pmutils_bin = "pm-suspend-hybrid";
+ break;
+ default:
+ error_setg(errp, "unknown guest suspend mode");
+ return;
+ }
+
+ pmutils_path = g_find_program_in_path(pmutils_bin);
+ if (!pmutils_path) {
+ error_setg(errp, "the helper program '%s' was not found", pmutils_bin);
+ return;
+ }
+
+ pid = fork();
+ if (!pid) {
+ setsid();
+ execle(pmutils_path, pmutils_bin, NULL, environ);
+ /*
+ * If we get here execle() has failed.
+ */
+ _exit(EXIT_FAILURE);
+ } else if (pid < 0) {
+ error_setg_errno(errp, errno, "failed to create child process");
+ goto out;
+ }
+
+ ga_wait_child(pid, &status, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ goto out;
+ }
+
+ if (WEXITSTATUS(status)) {
+ error_setg(errp,
+ "the helper program '%s' returned an unexpected exit status"
+ " code (%d)", pmutils_path, WEXITSTATUS(status));
+ }
+
+out:
+ g_free(pmutils_path);
+}
+
static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
{
const char *sysfile_str;
@@ -1545,64 +1604,28 @@ static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
return false;
}
-static void bios_supports_mode(int suspend_mode, Error **errp)
-{
- Error *local_err = NULL;
- bool ret;
-
- ret = pmutils_supports_mode(suspend_mode, &local_err);
- if (ret) {
- return;
- }
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
- ret = linux_sys_state_supports_mode(suspend_mode, errp);
- if (!ret) {
- error_setg(errp,
- "the requested suspend mode is not supported by the guest");
- return;
- }
-}
-
-static void guest_suspend(int suspend_mode, Error **errp)
+static void linux_sys_state_suspend(int suspend_mode, Error **errp)
{
Error *local_err = NULL;
- const char *pmutils_bin, *sysfile_str;
- char *pmutils_path;
+ const char *sysfile_str;
pid_t pid;
int status;
- bios_supports_mode(suspend_mode, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
-
switch (suspend_mode) {
case SUSPEND_MODE_DISK:
- pmutils_bin = "pm-hibernate";
sysfile_str = "disk";
break;
case SUSPEND_MODE_RAM:
- pmutils_bin = "pm-suspend";
sysfile_str = "mem";
break;
- case SUSPEND_MODE_HYBRID:
- pmutils_bin = "pm-suspend-hybrid";
- sysfile_str = NULL;
- break;
default:
error_setg(errp, "unknown guest suspend mode");
return;
}
- pmutils_path = g_find_program_in_path(pmutils_bin);
-
pid = fork();
- if (pid == 0) {
+ if (!pid) {
/* child */
int fd;
@@ -1611,19 +1634,6 @@ static void guest_suspend(int suspend_mode, Error **errp)
reopen_fd_to_null(1);
reopen_fd_to_null(2);
- if (pmutils_path) {
- execle(pmutils_path, pmutils_bin, NULL, environ);
- }
-
- /*
- * If we get here either pm-utils is not installed or execle() has
- * failed. Let's try the manual method if the caller wants it.
- */
-
- if (!sysfile_str) {
- _exit(EXIT_FAILURE);
- }
-
fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
if (fd < 0) {
_exit(EXIT_FAILURE);
@@ -1636,27 +1646,63 @@ static void guest_suspend(int suspend_mode, Error **errp)
_exit(EXIT_SUCCESS);
} else if (pid < 0) {
error_setg_errno(errp, errno, "failed to create child process");
- goto out;
+ return;
}
ga_wait_child(pid, &status, &local_err);
if (local_err) {
error_propagate(errp, local_err);
- goto out;
- }
-
- if (!WIFEXITED(status)) {
- error_setg(errp, "child process has terminated abnormally");
- goto out;
+ return;
}
if (WEXITSTATUS(status)) {
error_setg(errp, "child process has failed to suspend");
- goto out;
}
-out:
- g_free(pmutils_path);
+}
+
+static void bios_supports_mode(int suspend_mode, Error **errp)
+{
+ Error *local_err = NULL;
+ bool ret;
+
+ ret = pmutils_supports_mode(suspend_mode, &local_err);
+ if (ret) {
+ return;
+ }
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ ret = linux_sys_state_supports_mode(suspend_mode, errp);
+ if (!ret) {
+ error_setg(errp,
+ "the requested suspend mode is not supported by the guest");
+ return;
+ }
+}
+
+static void guest_suspend(int suspend_mode, Error **errp)
+{
+ Error *local_err = NULL;
+
+ bios_supports_mode(suspend_mode, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ pmutils_suspend(suspend_mode, &local_err);
+ if (!local_err) {
+ return;
+ }
+
+ local_err = NULL;
+
+ linux_sys_state_suspend(suspend_mode, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ }
}
void qmp_guest_suspend_disk(Error **errp)
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v1 3/6] qga: guest_suspend: decoupling pm-utils and sys logic
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 3/6] qga: guest_suspend: " Daniel Henrique Barboza
@ 2018-06-19 23:23 ` Marc-André Lureau
2018-06-20 20:05 ` Daniel Henrique Barboza
0 siblings, 1 reply; 12+ messages in thread
From: Marc-André Lureau @ 2018-06-19 23:23 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: QEMU, Markus Armbruster, Michael Roth
Hi
On Tue, Jun 19, 2018 at 9:38 PM, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
> Following the same logic of the previous patch, let's also
> decouple the suspend logic from guest_suspend into specialized
> functions, one for each strategy we support at this moment.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> qga/commands-posix.c | 170 +++++++++++++++++++++++++++----------------
> 1 file changed, 108 insertions(+), 62 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 89ffd8dc88..a2870f9ab9 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1509,6 +1509,65 @@ out:
> return ret;
> }
>
> +static void pmutils_suspend(int suspend_mode, Error **errp)
> +{
> + Error *local_err = NULL;
> + const char *pmutils_bin;
> + char *pmutils_path;
> + pid_t pid;
> + int status;
> +
> + switch (suspend_mode) {
> +
> + case SUSPEND_MODE_DISK:
> + pmutils_bin = "pm-hibernate";
> + break;
> + case SUSPEND_MODE_RAM:
> + pmutils_bin = "pm-suspend";
> + break;
> + case SUSPEND_MODE_HYBRID:
> + pmutils_bin = "pm-suspend-hybrid";
> + break;
> + default:
> + error_setg(errp, "unknown guest suspend mode");
> + return;
> + }
> +
> + pmutils_path = g_find_program_in_path(pmutils_bin);
> + if (!pmutils_path) {
> + error_setg(errp, "the helper program '%s' was not found", pmutils_bin);
> + return;
> + }
> +
> + pid = fork();
> + if (!pid) {
> + setsid();
> + execle(pmutils_path, pmutils_bin, NULL, environ);
> + /*
> + * If we get here execle() has failed.
> + */
> + _exit(EXIT_FAILURE);
> + } else if (pid < 0) {
> + error_setg_errno(errp, errno, "failed to create child process");
> + goto out;
> + }
> +
> + ga_wait_child(pid, &status, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + goto out;
> + }
> +
> + if (WEXITSTATUS(status)) {
> + error_setg(errp,
> + "the helper program '%s' returned an unexpected exit status"
> + " code (%d)", pmutils_path, WEXITSTATUS(status));
> + }
> +
> +out:
> + g_free(pmutils_path);
> +}
> +
> static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
> {
> const char *sysfile_str;
> @@ -1545,64 +1604,28 @@ static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
> return false;
> }
>
> -static void bios_supports_mode(int suspend_mode, Error **errp)
> -{
> - Error *local_err = NULL;
> - bool ret;
> -
> - ret = pmutils_supports_mode(suspend_mode, &local_err);
> - if (ret) {
> - return;
> - }
> - if (local_err) {
> - error_propagate(errp, local_err);
> - return;
> - }
> - ret = linux_sys_state_supports_mode(suspend_mode, errp);
> - if (!ret) {
> - error_setg(errp,
> - "the requested suspend mode is not supported by the guest");
> - return;
> - }
> -}
> -
> -static void guest_suspend(int suspend_mode, Error **errp)
> +static void linux_sys_state_suspend(int suspend_mode, Error **errp)
> {
> Error *local_err = NULL;
> - const char *pmutils_bin, *sysfile_str;
> - char *pmutils_path;
> + const char *sysfile_str;
> pid_t pid;
> int status;
>
> - bios_supports_mode(suspend_mode, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - return;
> - }
> -
> switch (suspend_mode) {
>
> case SUSPEND_MODE_DISK:
> - pmutils_bin = "pm-hibernate";
> sysfile_str = "disk";
> break;
> case SUSPEND_MODE_RAM:
> - pmutils_bin = "pm-suspend";
> sysfile_str = "mem";
> break;
> - case SUSPEND_MODE_HYBRID:
> - pmutils_bin = "pm-suspend-hybrid";
> - sysfile_str = NULL;
> - break;
> default:
> error_setg(errp, "unknown guest suspend mode");
> return;
> }
>
> - pmutils_path = g_find_program_in_path(pmutils_bin);
> -
> pid = fork();
> - if (pid == 0) {
> + if (!pid) {
> /* child */
> int fd;
>
> @@ -1611,19 +1634,6 @@ static void guest_suspend(int suspend_mode, Error **errp)
> reopen_fd_to_null(1);
> reopen_fd_to_null(2);
>
> - if (pmutils_path) {
> - execle(pmutils_path, pmutils_bin, NULL, environ);
> - }
> -
> - /*
> - * If we get here either pm-utils is not installed or execle() has
> - * failed. Let's try the manual method if the caller wants it.
> - */
> -
> - if (!sysfile_str) {
> - _exit(EXIT_FAILURE);
> - }
> -
> fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> if (fd < 0) {
> _exit(EXIT_FAILURE);
> @@ -1636,27 +1646,63 @@ static void guest_suspend(int suspend_mode, Error **errp)
> _exit(EXIT_SUCCESS);
> } else if (pid < 0) {
> error_setg_errno(errp, errno, "failed to create child process");
> - goto out;
> + return;
> }
>
> ga_wait_child(pid, &status, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> - goto out;
> - }
> -
> - if (!WIFEXITED(status)) {
> - error_setg(errp, "child process has terminated abnormally");
> - goto out;
> + return;
> }
>
> if (WEXITSTATUS(status)) {
> error_setg(errp, "child process has failed to suspend");
> - goto out;
> }
>
> -out:
> - g_free(pmutils_path);
> +}
> +
> +static void bios_supports_mode(int suspend_mode, Error **errp)
> +{
> + Error *local_err = NULL;
> + bool ret;
> +
> + ret = pmutils_supports_mode(suspend_mode, &local_err);
> + if (ret) {
> + return;
> + }
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + ret = linux_sys_state_supports_mode(suspend_mode, errp);
> + if (!ret) {
> + error_setg(errp,
> + "the requested suspend mode is not supported by the guest");
> + return;
> + }
> +}
> +
> +static void guest_suspend(int suspend_mode, Error **errp)
> +{
> + Error *local_err = NULL;
> +
> + bios_supports_mode(suspend_mode, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + pmutils_suspend(suspend_mode, &local_err);
> + if (!local_err) {
> + return;
> + }
> +
> + local_err = NULL;
You should error_free(). Same in later patches.
> +
> + linux_sys_state_suspend(suspend_mode, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + }
> }
>
> void qmp_guest_suspend_disk(Error **errp)
> --
> 2.17.1
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v1 3/6] qga: guest_suspend: decoupling pm-utils and sys logic
2018-06-19 23:23 ` Marc-André Lureau
@ 2018-06-20 20:05 ` Daniel Henrique Barboza
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Henrique Barboza @ 2018-06-20 20:05 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: QEMU, Markus Armbruster, Michael Roth
Hi,
On 06/19/2018 08:23 PM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Jun 19, 2018 at 9:38 PM, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>> Following the same logic of the previous patch, let's also
>> decouple the suspend logic from guest_suspend into specialized
>> functions, one for each strategy we support at this moment.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> qga/commands-posix.c | 170 +++++++++++++++++++++++++++----------------
>> 1 file changed, 108 insertions(+), 62 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 89ffd8dc88..a2870f9ab9 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1509,6 +1509,65 @@ out:
>> return ret;
>> }
>>
>> +static void pmutils_suspend(int suspend_mode, Error **errp)
>> +{
>> + Error *local_err = NULL;
>> + const char *pmutils_bin;
>> + char *pmutils_path;
>> + pid_t pid;
>> + int status;
>> +
>> + switch (suspend_mode) {
>> +
>> + case SUSPEND_MODE_DISK:
>> + pmutils_bin = "pm-hibernate";
>> + break;
>> + case SUSPEND_MODE_RAM:
>> + pmutils_bin = "pm-suspend";
>> + break;
>> + case SUSPEND_MODE_HYBRID:
>> + pmutils_bin = "pm-suspend-hybrid";
>> + break;
>> + default:
>> + error_setg(errp, "unknown guest suspend mode");
>> + return;
>> + }
>> +
>> + pmutils_path = g_find_program_in_path(pmutils_bin);
>> + if (!pmutils_path) {
>> + error_setg(errp, "the helper program '%s' was not found", pmutils_bin);
>> + return;
>> + }
>> +
>> + pid = fork();
>> + if (!pid) {
>> + setsid();
>> + execle(pmutils_path, pmutils_bin, NULL, environ);
>> + /*
>> + * If we get here execle() has failed.
>> + */
>> + _exit(EXIT_FAILURE);
>> + } else if (pid < 0) {
>> + error_setg_errno(errp, errno, "failed to create child process");
>> + goto out;
>> + }
>> +
>> + ga_wait_child(pid, &status, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + goto out;
>> + }
>> +
>> + if (WEXITSTATUS(status)) {
>> + error_setg(errp,
>> + "the helper program '%s' returned an unexpected exit status"
>> + " code (%d)", pmutils_path, WEXITSTATUS(status));
>> + }
>> +
>> +out:
>> + g_free(pmutils_path);
>> +}
>> +
>> static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
>> {
>> const char *sysfile_str;
>> @@ -1545,64 +1604,28 @@ static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
>> return false;
>> }
>>
>> -static void bios_supports_mode(int suspend_mode, Error **errp)
>> -{
>> - Error *local_err = NULL;
>> - bool ret;
>> -
>> - ret = pmutils_supports_mode(suspend_mode, &local_err);
>> - if (ret) {
>> - return;
>> - }
>> - if (local_err) {
>> - error_propagate(errp, local_err);
>> - return;
>> - }
>> - ret = linux_sys_state_supports_mode(suspend_mode, errp);
>> - if (!ret) {
>> - error_setg(errp,
>> - "the requested suspend mode is not supported by the guest");
>> - return;
>> - }
>> -}
>> -
>> -static void guest_suspend(int suspend_mode, Error **errp)
>> +static void linux_sys_state_suspend(int suspend_mode, Error **errp)
>> {
>> Error *local_err = NULL;
>> - const char *pmutils_bin, *sysfile_str;
>> - char *pmutils_path;
>> + const char *sysfile_str;
>> pid_t pid;
>> int status;
>>
>> - bios_supports_mode(suspend_mode, &local_err);
>> - if (local_err) {
>> - error_propagate(errp, local_err);
>> - return;
>> - }
>> -
>> switch (suspend_mode) {
>>
>> case SUSPEND_MODE_DISK:
>> - pmutils_bin = "pm-hibernate";
>> sysfile_str = "disk";
>> break;
>> case SUSPEND_MODE_RAM:
>> - pmutils_bin = "pm-suspend";
>> sysfile_str = "mem";
>> break;
>> - case SUSPEND_MODE_HYBRID:
>> - pmutils_bin = "pm-suspend-hybrid";
>> - sysfile_str = NULL;
>> - break;
>> default:
>> error_setg(errp, "unknown guest suspend mode");
>> return;
>> }
>>
>> - pmutils_path = g_find_program_in_path(pmutils_bin);
>> -
>> pid = fork();
>> - if (pid == 0) {
>> + if (!pid) {
>> /* child */
>> int fd;
>>
>> @@ -1611,19 +1634,6 @@ static void guest_suspend(int suspend_mode, Error **errp)
>> reopen_fd_to_null(1);
>> reopen_fd_to_null(2);
>>
>> - if (pmutils_path) {
>> - execle(pmutils_path, pmutils_bin, NULL, environ);
>> - }
>> -
>> - /*
>> - * If we get here either pm-utils is not installed or execle() has
>> - * failed. Let's try the manual method if the caller wants it.
>> - */
>> -
>> - if (!sysfile_str) {
>> - _exit(EXIT_FAILURE);
>> - }
>> -
>> fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
>> if (fd < 0) {
>> _exit(EXIT_FAILURE);
>> @@ -1636,27 +1646,63 @@ static void guest_suspend(int suspend_mode, Error **errp)
>> _exit(EXIT_SUCCESS);
>> } else if (pid < 0) {
>> error_setg_errno(errp, errno, "failed to create child process");
>> - goto out;
>> + return;
>> }
>>
>> ga_wait_child(pid, &status, &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> - goto out;
>> - }
>> -
>> - if (!WIFEXITED(status)) {
>> - error_setg(errp, "child process has terminated abnormally");
>> - goto out;
>> + return;
>> }
>>
>> if (WEXITSTATUS(status)) {
>> error_setg(errp, "child process has failed to suspend");
>> - goto out;
>> }
>>
>> -out:
>> - g_free(pmutils_path);
>> +}
>> +
>> +static void bios_supports_mode(int suspend_mode, Error **errp)
>> +{
>> + Error *local_err = NULL;
>> + bool ret;
>> +
>> + ret = pmutils_supports_mode(suspend_mode, &local_err);
>> + if (ret) {
>> + return;
>> + }
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> + ret = linux_sys_state_supports_mode(suspend_mode, errp);
>> + if (!ret) {
>> + error_setg(errp,
>> + "the requested suspend mode is not supported by the guest");
>> + return;
>> + }
>> +}
>> +
>> +static void guest_suspend(int suspend_mode, Error **errp)
>> +{
>> + Error *local_err = NULL;
>> +
>> + bios_supports_mode(suspend_mode, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> +
>> + pmutils_suspend(suspend_mode, &local_err);
>> + if (!local_err) {
>> + return;
>> + }
>> +
>> + local_err = NULL;
> You should error_free(). Same in later patches.
Thanks, I've fixed it for v2.
Daniel
>
>> +
>> + linux_sys_state_suspend(suspend_mode, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + }
>> }
>>
>> void qmp_guest_suspend_disk(Error **errp)
>> --
>> 2.17.1
>>
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v1 4/6] qga: removing switch statements, adding run_process_child
2018-06-19 19:38 [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep Daniel Henrique Barboza
` (2 preceding siblings ...)
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 3/6] qga: guest_suspend: " Daniel Henrique Barboza
@ 2018-06-19 19:38 ` Daniel Henrique Barboza
2018-06-19 23:25 ` Marc-André Lureau
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 5/6] qga: adding systemd hibernate/suspend/hybrid-sleep support Daniel Henrique Barboza
` (2 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: Daniel Henrique Barboza @ 2018-06-19 19:38 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, mdroth, Daniel Henrique Barboza
This is a cleanup of the resulting code after detaching
pmutils and Linux sys state file logic:
- remove the SUSPEND_MODE_* macros and use an enumeration
instead. At the same time, drop the switch statements
at the start of each function and use the enumeration
index to get the right binary/argument;
- create a new function called run_process_child(). This
function creates a child process and executes a (shell)
command, returning the command return code. This is a common
operation in the pmutils functions and will be used in the
systemd implementation as well, so this function will avoid
code repetition.
There are more places inside commands-posix.c where this new
run_process_child function can also be used, but one step
at a time.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
qga/commands-posix.c | 190 +++++++++++++++++--------------------------
1 file changed, 76 insertions(+), 114 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index a2870f9ab9..d5e3805ce9 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1438,152 +1438,122 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
#define LINUX_SYS_STATE_FILE "/sys/power/state"
#define SUSPEND_SUPPORTED 0
#define SUSPEND_NOT_SUPPORTED 1
-#define SUSPEND_MODE_DISK 1
-#define SUSPEND_MODE_RAM 2
-#define SUSPEND_MODE_HYBRID 3
-static bool pmutils_supports_mode(int suspend_mode, Error **errp)
+typedef enum {
+ SUSPEND_MODE_DISK = 0,
+ SUSPEND_MODE_RAM = 1,
+ SUSPEND_MODE_HYBRID = 2,
+} SuspendMode;
+
+static int run_process_child(const char *command[], Error **errp)
{
Error *local_err = NULL;
- const char *pmutils_arg;
- const char *pmutils_bin = "pm-is-supported";
- char *pmutils_path;
+ char *cmd_path = g_find_program_in_path(command[0]);
pid_t pid;
- int status;
- bool ret = false;
-
- switch (suspend_mode) {
-
- case SUSPEND_MODE_DISK:
- pmutils_arg = "--hibernate";
- break;
- case SUSPEND_MODE_RAM:
- pmutils_arg = "--suspend";
- break;
- case SUSPEND_MODE_HYBRID:
- pmutils_arg = "--suspend-hybrid";
- break;
- default:
- return ret;
- }
+ int status, ret = -1;
- pmutils_path = g_find_program_in_path(pmutils_bin);
- if (!pmutils_path) {
+ if (!cmd_path) {
return ret;
}
pid = fork();
if (!pid) {
setsid();
- execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
/*
- * If we get here execle() has failed.
+ * execve receives a char* const argv[] as second arg but we're
+ * receiving a const char*[]. Since execve does not change the
+ * array contents it's tolerable to cast here.
*/
- _exit(SUSPEND_NOT_SUPPORTED);
+ execve(cmd_path, (char* const*)command, environ);
+ _exit(errno);
} else if (pid < 0) {
error_setg_errno(errp, errno, "failed to create child process");
+ ret = EXIT_FAILURE;
goto out;
}
ga_wait_child(pid, &status, &local_err);
if (local_err) {
error_propagate(errp, local_err);
+ ret = EXIT_FAILURE;
goto out;
}
- switch (WEXITSTATUS(status)) {
- case SUSPEND_SUPPORTED:
- ret = true;
- goto out;
- case SUSPEND_NOT_SUPPORTED:
- goto out;
- default:
- error_setg(errp,
- "the helper program '%s' returned an unexpected exit status"
- " code (%d)", pmutils_path, WEXITSTATUS(status));
- goto out;
- }
+ ret = WEXITSTATUS(status);
out:
- g_free(pmutils_path);
+ g_free(cmd_path);
return ret;
}
-static void pmutils_suspend(int suspend_mode, Error **errp)
+static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
{
Error *local_err = NULL;
- const char *pmutils_bin;
- char *pmutils_path;
- pid_t pid;
+ const char *pmutils_args[3] = {"--hibernate", "--suspend",
+ "--suspend-hybrid"};
+ const char *cmd[3] = {"pm-is-supported", pmutils_args[mode], NULL};
int status;
- switch (suspend_mode) {
-
- case SUSPEND_MODE_DISK:
- pmutils_bin = "pm-hibernate";
- break;
- case SUSPEND_MODE_RAM:
- pmutils_bin = "pm-suspend";
- break;
- case SUSPEND_MODE_HYBRID:
- pmutils_bin = "pm-suspend-hybrid";
- break;
- default:
- error_setg(errp, "unknown guest suspend mode");
- return;
- }
+ status = run_process_child(cmd, &local_err);
- pmutils_path = g_find_program_in_path(pmutils_bin);
- if (!pmutils_path) {
- error_setg(errp, "the helper program '%s' was not found", pmutils_bin);
- return;
+ if (status == SUSPEND_SUPPORTED) {
+ return true;
}
- pid = fork();
- if (!pid) {
- setsid();
- execle(pmutils_path, pmutils_bin, NULL, environ);
- /*
- * If we get here execle() has failed.
- */
- _exit(EXIT_FAILURE);
- } else if (pid < 0) {
- error_setg_errno(errp, errno, "failed to create child process");
- goto out;
+ if (status == -1) {
+ return false;
}
- ga_wait_child(pid, &status, &local_err);
if (local_err) {
error_propagate(errp, local_err);
- goto out;
+ } else {
+ error_setg(errp,
+ "the helper program '%s' returned an unexpected exit"
+ " status code (%d)", "pm-is-supported", status);
}
- if (WEXITSTATUS(status)) {
- error_setg(errp,
- "the helper program '%s' returned an unexpected exit status"
- " code (%d)", pmutils_path, WEXITSTATUS(status));
+ return false;
+}
+
+static void pmutils_suspend(SuspendMode mode, Error **errp)
+{
+ Error *local_err = NULL;
+ const char *pmutils_binaries[3] = {"pm-hibernate", "pm-suspend",
+ "pm-suspend-hybrid"};
+ const char *cmd[2] = {pmutils_binaries[mode], NULL};
+ int status;
+
+ status = run_process_child(cmd, &local_err);
+
+ if (status == 0) {
+ return;
}
-out:
- g_free(pmutils_path);
+ if (status == -1) {
+ error_setg(errp, "the helper program '%s' was not found",
+ pmutils_binaries[mode]);
+ return;
+ }
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ } else {
+ error_setg(errp,
+ "the helper program '%s' returned an unexpected exit"
+ " status code (%d)", pmutils_binaries[mode], status);
+ }
}
-static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
+static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
{
- const char *sysfile_str;
+ const char *sysfile_strs[3] = {"disk", "mem", NULL};
+ const char *sysfile_str = sysfile_strs[mode];
char buf[32]; /* hopefully big enough */
int fd;
ssize_t ret;
- switch (suspend_mode) {
-
- case SUSPEND_MODE_DISK:
- sysfile_str = "disk";
- break;
- case SUSPEND_MODE_RAM:
- sysfile_str = "mem";
- break;
- default:
+ if (!sysfile_str) {
+ error_setg(errp, "unknown guest suspend mode");
return false;
}
@@ -1604,22 +1574,15 @@ static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
return false;
}
-static void linux_sys_state_suspend(int suspend_mode, Error **errp)
+static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
{
Error *local_err = NULL;
- const char *sysfile_str;
+ const char *sysfile_strs[3] = {"disk", "mem", NULL};
+ const char *sysfile_str = sysfile_strs[mode];
pid_t pid;
int status;
- switch (suspend_mode) {
-
- case SUSPEND_MODE_DISK:
- sysfile_str = "disk";
- break;
- case SUSPEND_MODE_RAM:
- sysfile_str = "mem";
- break;
- default:
+ if (!sysfile_str) {
error_setg(errp, "unknown guest suspend mode");
return;
}
@@ -1661,12 +1624,12 @@ static void linux_sys_state_suspend(int suspend_mode, Error **errp)
}
-static void bios_supports_mode(int suspend_mode, Error **errp)
+static void bios_supports_mode(SuspendMode mode, Error **errp)
{
Error *local_err = NULL;
bool ret;
- ret = pmutils_supports_mode(suspend_mode, &local_err);
+ ret = pmutils_supports_mode(mode, &local_err);
if (ret) {
return;
}
@@ -1674,32 +1637,31 @@ static void bios_supports_mode(int suspend_mode, Error **errp)
error_propagate(errp, local_err);
return;
}
- ret = linux_sys_state_supports_mode(suspend_mode, errp);
+ ret = linux_sys_state_supports_mode(mode, &local_err);
if (!ret) {
error_setg(errp,
"the requested suspend mode is not supported by the guest");
- return;
}
}
-static void guest_suspend(int suspend_mode, Error **errp)
+static void guest_suspend(SuspendMode mode, Error **errp)
{
Error *local_err = NULL;
- bios_supports_mode(suspend_mode, &local_err);
+ bios_supports_mode(mode, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
- pmutils_suspend(suspend_mode, &local_err);
+ pmutils_suspend(mode, &local_err);
if (!local_err) {
return;
}
local_err = NULL;
- linux_sys_state_suspend(suspend_mode, &local_err);
+ linux_sys_state_suspend(mode, &local_err);
if (local_err) {
error_propagate(errp, local_err);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v1 4/6] qga: removing switch statements, adding run_process_child
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 4/6] qga: removing switch statements, adding run_process_child Daniel Henrique Barboza
@ 2018-06-19 23:25 ` Marc-André Lureau
2018-06-20 20:10 ` Daniel Henrique Barboza
0 siblings, 1 reply; 12+ messages in thread
From: Marc-André Lureau @ 2018-06-19 23:25 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: QEMU, Markus Armbruster, Michael Roth
Hi
On Tue, Jun 19, 2018 at 9:38 PM, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
> This is a cleanup of the resulting code after detaching
> pmutils and Linux sys state file logic:
>
> - remove the SUSPEND_MODE_* macros and use an enumeration
> instead. At the same time, drop the switch statements
> at the start of each function and use the enumeration
> index to get the right binary/argument;
>
> - create a new function called run_process_child(). This
> function creates a child process and executes a (shell)
> command, returning the command return code. This is a common
What about using g_spawn_sync() instead?
> operation in the pmutils functions and will be used in the
> systemd implementation as well, so this function will avoid
> code repetition.
>
> There are more places inside commands-posix.c where this new
> run_process_child function can also be used, but one step
> at a time.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> qga/commands-posix.c | 190 +++++++++++++++++--------------------------
> 1 file changed, 76 insertions(+), 114 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a2870f9ab9..d5e3805ce9 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1438,152 +1438,122 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
> #define LINUX_SYS_STATE_FILE "/sys/power/state"
> #define SUSPEND_SUPPORTED 0
> #define SUSPEND_NOT_SUPPORTED 1
> -#define SUSPEND_MODE_DISK 1
> -#define SUSPEND_MODE_RAM 2
> -#define SUSPEND_MODE_HYBRID 3
>
> -static bool pmutils_supports_mode(int suspend_mode, Error **errp)
> +typedef enum {
> + SUSPEND_MODE_DISK = 0,
> + SUSPEND_MODE_RAM = 1,
> + SUSPEND_MODE_HYBRID = 2,
> +} SuspendMode;
> +
> +static int run_process_child(const char *command[], Error **errp)
> {
> Error *local_err = NULL;
> - const char *pmutils_arg;
> - const char *pmutils_bin = "pm-is-supported";
> - char *pmutils_path;
> + char *cmd_path = g_find_program_in_path(command[0]);
> pid_t pid;
> - int status;
> - bool ret = false;
> -
> - switch (suspend_mode) {
> -
> - case SUSPEND_MODE_DISK:
> - pmutils_arg = "--hibernate";
> - break;
> - case SUSPEND_MODE_RAM:
> - pmutils_arg = "--suspend";
> - break;
> - case SUSPEND_MODE_HYBRID:
> - pmutils_arg = "--suspend-hybrid";
> - break;
> - default:
> - return ret;
> - }
> + int status, ret = -1;
>
> - pmutils_path = g_find_program_in_path(pmutils_bin);
> - if (!pmutils_path) {
> + if (!cmd_path) {
> return ret;
> }
>
> pid = fork();
> if (!pid) {
> setsid();
> - execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
> /*
> - * If we get here execle() has failed.
> + * execve receives a char* const argv[] as second arg but we're
> + * receiving a const char*[]. Since execve does not change the
> + * array contents it's tolerable to cast here.
> */
> - _exit(SUSPEND_NOT_SUPPORTED);
> + execve(cmd_path, (char* const*)command, environ);
> + _exit(errno);
> } else if (pid < 0) {
> error_setg_errno(errp, errno, "failed to create child process");
> + ret = EXIT_FAILURE;
> goto out;
> }
>
> ga_wait_child(pid, &status, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> + ret = EXIT_FAILURE;
> goto out;
> }
>
> - switch (WEXITSTATUS(status)) {
> - case SUSPEND_SUPPORTED:
> - ret = true;
> - goto out;
> - case SUSPEND_NOT_SUPPORTED:
> - goto out;
> - default:
> - error_setg(errp,
> - "the helper program '%s' returned an unexpected exit status"
> - " code (%d)", pmutils_path, WEXITSTATUS(status));
> - goto out;
> - }
> + ret = WEXITSTATUS(status);
>
> out:
> - g_free(pmutils_path);
> + g_free(cmd_path);
> return ret;
> }
>
> -static void pmutils_suspend(int suspend_mode, Error **errp)
> +static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
> {
> Error *local_err = NULL;
> - const char *pmutils_bin;
> - char *pmutils_path;
> - pid_t pid;
> + const char *pmutils_args[3] = {"--hibernate", "--suspend",
> + "--suspend-hybrid"};
> + const char *cmd[3] = {"pm-is-supported", pmutils_args[mode], NULL};
> int status;
>
> - switch (suspend_mode) {
> -
> - case SUSPEND_MODE_DISK:
> - pmutils_bin = "pm-hibernate";
> - break;
> - case SUSPEND_MODE_RAM:
> - pmutils_bin = "pm-suspend";
> - break;
> - case SUSPEND_MODE_HYBRID:
> - pmutils_bin = "pm-suspend-hybrid";
> - break;
> - default:
> - error_setg(errp, "unknown guest suspend mode");
> - return;
> - }
> + status = run_process_child(cmd, &local_err);
>
> - pmutils_path = g_find_program_in_path(pmutils_bin);
> - if (!pmutils_path) {
> - error_setg(errp, "the helper program '%s' was not found", pmutils_bin);
> - return;
> + if (status == SUSPEND_SUPPORTED) {
> + return true;
> }
>
> - pid = fork();
> - if (!pid) {
> - setsid();
> - execle(pmutils_path, pmutils_bin, NULL, environ);
> - /*
> - * If we get here execle() has failed.
> - */
> - _exit(EXIT_FAILURE);
> - } else if (pid < 0) {
> - error_setg_errno(errp, errno, "failed to create child process");
> - goto out;
> + if (status == -1) {
> + return false;
> }
>
> - ga_wait_child(pid, &status, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> - goto out;
> + } else {
> + error_setg(errp,
> + "the helper program '%s' returned an unexpected exit"
> + " status code (%d)", "pm-is-supported", status);
> }
>
> - if (WEXITSTATUS(status)) {
> - error_setg(errp,
> - "the helper program '%s' returned an unexpected exit status"
> - " code (%d)", pmutils_path, WEXITSTATUS(status));
> + return false;
> +}
> +
> +static void pmutils_suspend(SuspendMode mode, Error **errp)
> +{
> + Error *local_err = NULL;
> + const char *pmutils_binaries[3] = {"pm-hibernate", "pm-suspend",
> + "pm-suspend-hybrid"};
> + const char *cmd[2] = {pmutils_binaries[mode], NULL};
> + int status;
> +
> + status = run_process_child(cmd, &local_err);
> +
> + if (status == 0) {
> + return;
> }
>
> -out:
> - g_free(pmutils_path);
> + if (status == -1) {
> + error_setg(errp, "the helper program '%s' was not found",
> + pmutils_binaries[mode]);
> + return;
> + }
> +
> + if (local_err) {
> + error_propagate(errp, local_err);
> + } else {
> + error_setg(errp,
> + "the helper program '%s' returned an unexpected exit"
> + " status code (%d)", pmutils_binaries[mode], status);
> + }
> }
>
> -static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
> +static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
> {
> - const char *sysfile_str;
> + const char *sysfile_strs[3] = {"disk", "mem", NULL};
> + const char *sysfile_str = sysfile_strs[mode];
> char buf[32]; /* hopefully big enough */
> int fd;
> ssize_t ret;
>
> - switch (suspend_mode) {
> -
> - case SUSPEND_MODE_DISK:
> - sysfile_str = "disk";
> - break;
> - case SUSPEND_MODE_RAM:
> - sysfile_str = "mem";
> - break;
> - default:
> + if (!sysfile_str) {
> + error_setg(errp, "unknown guest suspend mode");
> return false;
> }
>
> @@ -1604,22 +1574,15 @@ static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
> return false;
> }
>
> -static void linux_sys_state_suspend(int suspend_mode, Error **errp)
> +static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
> {
> Error *local_err = NULL;
> - const char *sysfile_str;
> + const char *sysfile_strs[3] = {"disk", "mem", NULL};
> + const char *sysfile_str = sysfile_strs[mode];
> pid_t pid;
> int status;
>
> - switch (suspend_mode) {
> -
> - case SUSPEND_MODE_DISK:
> - sysfile_str = "disk";
> - break;
> - case SUSPEND_MODE_RAM:
> - sysfile_str = "mem";
> - break;
> - default:
> + if (!sysfile_str) {
> error_setg(errp, "unknown guest suspend mode");
> return;
> }
> @@ -1661,12 +1624,12 @@ static void linux_sys_state_suspend(int suspend_mode, Error **errp)
>
> }
>
> -static void bios_supports_mode(int suspend_mode, Error **errp)
> +static void bios_supports_mode(SuspendMode mode, Error **errp)
> {
> Error *local_err = NULL;
> bool ret;
>
> - ret = pmutils_supports_mode(suspend_mode, &local_err);
> + ret = pmutils_supports_mode(mode, &local_err);
> if (ret) {
> return;
> }
> @@ -1674,32 +1637,31 @@ static void bios_supports_mode(int suspend_mode, Error **errp)
> error_propagate(errp, local_err);
> return;
> }
> - ret = linux_sys_state_supports_mode(suspend_mode, errp);
> + ret = linux_sys_state_supports_mode(mode, &local_err);
> if (!ret) {
> error_setg(errp,
> "the requested suspend mode is not supported by the guest");
> - return;
> }
> }
>
> -static void guest_suspend(int suspend_mode, Error **errp)
> +static void guest_suspend(SuspendMode mode, Error **errp)
> {
> Error *local_err = NULL;
>
> - bios_supports_mode(suspend_mode, &local_err);
> + bios_supports_mode(mode, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
>
> - pmutils_suspend(suspend_mode, &local_err);
> + pmutils_suspend(mode, &local_err);
> if (!local_err) {
> return;
> }
>
> local_err = NULL;
>
> - linux_sys_state_suspend(suspend_mode, &local_err);
> + linux_sys_state_suspend(mode, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> }
> --
> 2.17.1
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v1 4/6] qga: removing switch statements, adding run_process_child
2018-06-19 23:25 ` Marc-André Lureau
@ 2018-06-20 20:10 ` Daniel Henrique Barboza
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Henrique Barboza @ 2018-06-20 20:10 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: QEMU, Markus Armbruster, Michael Roth
On 06/19/2018 08:25 PM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Jun 19, 2018 at 9:38 PM, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>> This is a cleanup of the resulting code after detaching
>> pmutils and Linux sys state file logic:
>>
>> - remove the SUSPEND_MODE_* macros and use an enumeration
>> instead. At the same time, drop the switch statements
>> at the start of each function and use the enumeration
>> index to get the right binary/argument;
>>
>> - create a new function called run_process_child(). This
>> function creates a child process and executes a (shell)
>> command, returning the command return code. This is a common
> What about using g_spawn_sync() instead?
Nice. I've rewritten run_process_child to work with g_spawn_sync(), sparing
the function from all the code to manage the fork() call and etc.
I'll test it a bit more and then send it in v2.
Thanks,
Daniel
>
>> operation in the pmutils functions and will be used in the
>> systemd implementation as well, so this function will avoid
>> code repetition.
>>
>> There are more places inside commands-posix.c where this new
>> run_process_child function can also be used, but one step
>> at a time.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> qga/commands-posix.c | 190 +++++++++++++++++--------------------------
>> 1 file changed, 76 insertions(+), 114 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index a2870f9ab9..d5e3805ce9 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1438,152 +1438,122 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>> #define LINUX_SYS_STATE_FILE "/sys/power/state"
>> #define SUSPEND_SUPPORTED 0
>> #define SUSPEND_NOT_SUPPORTED 1
>> -#define SUSPEND_MODE_DISK 1
>> -#define SUSPEND_MODE_RAM 2
>> -#define SUSPEND_MODE_HYBRID 3
>>
>> -static bool pmutils_supports_mode(int suspend_mode, Error **errp)
>> +typedef enum {
>> + SUSPEND_MODE_DISK = 0,
>> + SUSPEND_MODE_RAM = 1,
>> + SUSPEND_MODE_HYBRID = 2,
>> +} SuspendMode;
>> +
>> +static int run_process_child(const char *command[], Error **errp)
>> {
>> Error *local_err = NULL;
>> - const char *pmutils_arg;
>> - const char *pmutils_bin = "pm-is-supported";
>> - char *pmutils_path;
>> + char *cmd_path = g_find_program_in_path(command[0]);
>> pid_t pid;
>> - int status;
>> - bool ret = false;
>> -
>> - switch (suspend_mode) {
>> -
>> - case SUSPEND_MODE_DISK:
>> - pmutils_arg = "--hibernate";
>> - break;
>> - case SUSPEND_MODE_RAM:
>> - pmutils_arg = "--suspend";
>> - break;
>> - case SUSPEND_MODE_HYBRID:
>> - pmutils_arg = "--suspend-hybrid";
>> - break;
>> - default:
>> - return ret;
>> - }
>> + int status, ret = -1;
>>
>> - pmutils_path = g_find_program_in_path(pmutils_bin);
>> - if (!pmutils_path) {
>> + if (!cmd_path) {
>> return ret;
>> }
>>
>> pid = fork();
>> if (!pid) {
>> setsid();
>> - execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
>> /*
>> - * If we get here execle() has failed.
>> + * execve receives a char* const argv[] as second arg but we're
>> + * receiving a const char*[]. Since execve does not change the
>> + * array contents it's tolerable to cast here.
>> */
>> - _exit(SUSPEND_NOT_SUPPORTED);
>> + execve(cmd_path, (char* const*)command, environ);
>> + _exit(errno);
>> } else if (pid < 0) {
>> error_setg_errno(errp, errno, "failed to create child process");
>> + ret = EXIT_FAILURE;
>> goto out;
>> }
>>
>> ga_wait_child(pid, &status, &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> + ret = EXIT_FAILURE;
>> goto out;
>> }
>>
>> - switch (WEXITSTATUS(status)) {
>> - case SUSPEND_SUPPORTED:
>> - ret = true;
>> - goto out;
>> - case SUSPEND_NOT_SUPPORTED:
>> - goto out;
>> - default:
>> - error_setg(errp,
>> - "the helper program '%s' returned an unexpected exit status"
>> - " code (%d)", pmutils_path, WEXITSTATUS(status));
>> - goto out;
>> - }
>> + ret = WEXITSTATUS(status);
>>
>> out:
>> - g_free(pmutils_path);
>> + g_free(cmd_path);
>> return ret;
>> }
>>
>> -static void pmutils_suspend(int suspend_mode, Error **errp)
>> +static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
>> {
>> Error *local_err = NULL;
>> - const char *pmutils_bin;
>> - char *pmutils_path;
>> - pid_t pid;
>> + const char *pmutils_args[3] = {"--hibernate", "--suspend",
>> + "--suspend-hybrid"};
>> + const char *cmd[3] = {"pm-is-supported", pmutils_args[mode], NULL};
>> int status;
>>
>> - switch (suspend_mode) {
>> -
>> - case SUSPEND_MODE_DISK:
>> - pmutils_bin = "pm-hibernate";
>> - break;
>> - case SUSPEND_MODE_RAM:
>> - pmutils_bin = "pm-suspend";
>> - break;
>> - case SUSPEND_MODE_HYBRID:
>> - pmutils_bin = "pm-suspend-hybrid";
>> - break;
>> - default:
>> - error_setg(errp, "unknown guest suspend mode");
>> - return;
>> - }
>> + status = run_process_child(cmd, &local_err);
>>
>> - pmutils_path = g_find_program_in_path(pmutils_bin);
>> - if (!pmutils_path) {
>> - error_setg(errp, "the helper program '%s' was not found", pmutils_bin);
>> - return;
>> + if (status == SUSPEND_SUPPORTED) {
>> + return true;
>> }
>>
>> - pid = fork();
>> - if (!pid) {
>> - setsid();
>> - execle(pmutils_path, pmutils_bin, NULL, environ);
>> - /*
>> - * If we get here execle() has failed.
>> - */
>> - _exit(EXIT_FAILURE);
>> - } else if (pid < 0) {
>> - error_setg_errno(errp, errno, "failed to create child process");
>> - goto out;
>> + if (status == -1) {
>> + return false;
>> }
>>
>> - ga_wait_child(pid, &status, &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> - goto out;
>> + } else {
>> + error_setg(errp,
>> + "the helper program '%s' returned an unexpected exit"
>> + " status code (%d)", "pm-is-supported", status);
>> }
>>
>> - if (WEXITSTATUS(status)) {
>> - error_setg(errp,
>> - "the helper program '%s' returned an unexpected exit status"
>> - " code (%d)", pmutils_path, WEXITSTATUS(status));
>> + return false;
>> +}
>> +
>> +static void pmutils_suspend(SuspendMode mode, Error **errp)
>> +{
>> + Error *local_err = NULL;
>> + const char *pmutils_binaries[3] = {"pm-hibernate", "pm-suspend",
>> + "pm-suspend-hybrid"};
>> + const char *cmd[2] = {pmutils_binaries[mode], NULL};
>> + int status;
>> +
>> + status = run_process_child(cmd, &local_err);
>> +
>> + if (status == 0) {
>> + return;
>> }
>>
>> -out:
>> - g_free(pmutils_path);
>> + if (status == -1) {
>> + error_setg(errp, "the helper program '%s' was not found",
>> + pmutils_binaries[mode]);
>> + return;
>> + }
>> +
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + } else {
>> + error_setg(errp,
>> + "the helper program '%s' returned an unexpected exit"
>> + " status code (%d)", pmutils_binaries[mode], status);
>> + }
>> }
>>
>> -static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
>> +static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
>> {
>> - const char *sysfile_str;
>> + const char *sysfile_strs[3] = {"disk", "mem", NULL};
>> + const char *sysfile_str = sysfile_strs[mode];
>> char buf[32]; /* hopefully big enough */
>> int fd;
>> ssize_t ret;
>>
>> - switch (suspend_mode) {
>> -
>> - case SUSPEND_MODE_DISK:
>> - sysfile_str = "disk";
>> - break;
>> - case SUSPEND_MODE_RAM:
>> - sysfile_str = "mem";
>> - break;
>> - default:
>> + if (!sysfile_str) {
>> + error_setg(errp, "unknown guest suspend mode");
>> return false;
>> }
>>
>> @@ -1604,22 +1574,15 @@ static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
>> return false;
>> }
>>
>> -static void linux_sys_state_suspend(int suspend_mode, Error **errp)
>> +static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>> {
>> Error *local_err = NULL;
>> - const char *sysfile_str;
>> + const char *sysfile_strs[3] = {"disk", "mem", NULL};
>> + const char *sysfile_str = sysfile_strs[mode];
>> pid_t pid;
>> int status;
>>
>> - switch (suspend_mode) {
>> -
>> - case SUSPEND_MODE_DISK:
>> - sysfile_str = "disk";
>> - break;
>> - case SUSPEND_MODE_RAM:
>> - sysfile_str = "mem";
>> - break;
>> - default:
>> + if (!sysfile_str) {
>> error_setg(errp, "unknown guest suspend mode");
>> return;
>> }
>> @@ -1661,12 +1624,12 @@ static void linux_sys_state_suspend(int suspend_mode, Error **errp)
>>
>> }
>>
>> -static void bios_supports_mode(int suspend_mode, Error **errp)
>> +static void bios_supports_mode(SuspendMode mode, Error **errp)
>> {
>> Error *local_err = NULL;
>> bool ret;
>>
>> - ret = pmutils_supports_mode(suspend_mode, &local_err);
>> + ret = pmutils_supports_mode(mode, &local_err);
>> if (ret) {
>> return;
>> }
>> @@ -1674,32 +1637,31 @@ static void bios_supports_mode(int suspend_mode, Error **errp)
>> error_propagate(errp, local_err);
>> return;
>> }
>> - ret = linux_sys_state_supports_mode(suspend_mode, errp);
>> + ret = linux_sys_state_supports_mode(mode, &local_err);
>> if (!ret) {
>> error_setg(errp,
>> "the requested suspend mode is not supported by the guest");
>> - return;
>> }
>> }
>>
>> -static void guest_suspend(int suspend_mode, Error **errp)
>> +static void guest_suspend(SuspendMode mode, Error **errp)
>> {
>> Error *local_err = NULL;
>>
>> - bios_supports_mode(suspend_mode, &local_err);
>> + bios_supports_mode(mode, &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return;
>> }
>>
>> - pmutils_suspend(suspend_mode, &local_err);
>> + pmutils_suspend(mode, &local_err);
>> if (!local_err) {
>> return;
>> }
>>
>> local_err = NULL;
>>
>> - linux_sys_state_suspend(suspend_mode, &local_err);
>> + linux_sys_state_suspend(mode, &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> }
>> --
>> 2.17.1
>>
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v1 5/6] qga: adding systemd hibernate/suspend/hybrid-sleep support
2018-06-19 19:38 [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep Daniel Henrique Barboza
` (3 preceding siblings ...)
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 4/6] qga: removing switch statements, adding run_process_child Daniel Henrique Barboza
@ 2018-06-19 19:38 ` Daniel Henrique Barboza
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 6/6] qga: removing bios_supports_mode Daniel Henrique Barboza
2018-06-19 21:56 ` [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep no-reply
6 siblings, 0 replies; 12+ messages in thread
From: Daniel Henrique Barboza @ 2018-06-19 19:38 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, mdroth, Daniel Henrique Barboza
pmutils isn't being supported by newer OSes like Fedora 27
or Mint. This means that the only suspend option QGA offers
for these guests are writing directly into the Linux sys state
file. This also means that QGA also loses the ability to do
hybrid suspend in those guests - this suspend mode is only
available when using pmutils.
Newer guests can use systemd facilities to do all the suspend
times QGA supports. The mapping in comparison with pmutils is:
- pm-hibernate -> systemctl hibernate
- pm-suspend -> systemctl suspend
- pm-suspend-hybrid -> systemctl hybrid-sleep
To discover whether systemd supports these functions, we inspect
the status of the services that implements them.
With this patch, we can offer hybrid suspend again for newer
guests that do not have pmutils support anymore.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
qga/commands-posix.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 72 insertions(+)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index d5e3805ce9..6a573de86d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1486,6 +1486,63 @@ out:
return ret;
}
+static bool systemd_supports_mode(SuspendMode mode, Error **errp)
+{
+ Error *local_err = NULL;
+ const char *systemctl_args[3] = {"systemd-hibernate", "systemd-suspend",
+ "systemd-hybrid-sleep"};
+ const char *cmd[4] = {"systemctl", "status", systemctl_args[mode], NULL};
+ int status;
+
+ status = run_process_child(cmd, &local_err);
+
+ /*
+ * systemctl status uses LSB return codes so we can expect
+ * status > 0 and be ok. To assert if the guest has support
+ * for the selected suspend mode, status should be < 4. 4 is
+ * the code for unknown service status, the return value when
+ * the service does not exist. A common value is status = 3
+ * (program is not running).
+ */
+ if (status > 0 && status < 4) {
+ return true;
+ }
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ }
+
+ return false;
+}
+
+static void systemd_suspend(SuspendMode mode, Error **errp)
+{
+ Error *local_err = NULL;
+ const char *systemctl_args[3] = {"hibernate", "suspend", "hybrid-sleep"};
+ const char *cmd[3] = {"systemctl", systemctl_args[mode], NULL};
+ int status;
+
+ status = run_process_child(cmd, &local_err);
+
+ if (status == 0) {
+ return;
+ }
+
+ if (status == -1) {
+ error_setg(errp, "the helper program '%s' was not found",
+ systemctl_args[mode]);
+ return;
+ }
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ } else {
+ error_setg(errp, "the helper program 'systemctl %s' returned an "
+ " unexpected exit status code (%d)",
+ systemctl_args[mode], status);
+ }
+}
+
static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
{
Error *local_err = NULL;
@@ -1629,6 +1686,14 @@ static void bios_supports_mode(SuspendMode mode, Error **errp)
Error *local_err = NULL;
bool ret;
+ ret = systemd_supports_mode(mode, &local_err);
+ if (ret) {
+ return;
+ }
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
ret = pmutils_supports_mode(mode, &local_err);
if (ret) {
return;
@@ -1654,6 +1719,13 @@ static void guest_suspend(SuspendMode mode, Error **errp)
return;
}
+ systemd_suspend(mode, &local_err);
+ if (!local_err) {
+ return;
+ }
+
+ local_err = NULL;
+
pmutils_suspend(mode, &local_err);
if (!local_err) {
return;
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [Qemu-devel] [PATCH v1 6/6] qga: removing bios_supports_mode
2018-06-19 19:38 [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep Daniel Henrique Barboza
` (4 preceding siblings ...)
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 5/6] qga: adding systemd hibernate/suspend/hybrid-sleep support Daniel Henrique Barboza
@ 2018-06-19 19:38 ` Daniel Henrique Barboza
2018-06-19 21:56 ` [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep no-reply
6 siblings, 0 replies; 12+ messages in thread
From: Daniel Henrique Barboza @ 2018-06-19 19:38 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, mdroth, Daniel Henrique Barboza
bios_support_mode verifies if the guest has support for a certain
suspend mode but it doesn't inform back which suspend tool
provides it. The caller, guest_suspend, executes all suspend
strategies in order again.
After adding systemd suspend support, bios_support_mode now will
verify for support for systemd, then pmutils, then Linux sys state
file. In a worst case scenario where both systemd and pmutils isn't
supported but Linux sys state is:
- bios_supports_mode will check for systemd, then pmutils, then
Linux sys state. It will tell guest_suspend that there is support,
but it will not tell who provides it;
- guest_suspend will try to execute (and fail) systemd suspend,
then pmutils suspend, to only then use the Linux sys suspend.
The time spent executing systemd and pmutils suspend was wasted
and could be avoided, but only bios_support_mode knew it but
didn't inform it back.
A quicker approach is to nuke bios_supports_mode and control
whether we found support at all with a bool flag inside
guest_suspend. guest_suspend will search for suspend support
and execute it as soon as possible. If the a given suspend
mechanism fails, continue to the next. If no suspend
support is found, the "not supported" message is still being
sent back to the user.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
qga/commands-posix.c | 54 +++++++++++++++-----------------------------
1 file changed, 18 insertions(+), 36 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 6a573de86d..79acc28ee7 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1681,60 +1681,42 @@ static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
}
-static void bios_supports_mode(SuspendMode mode, Error **errp)
-{
- Error *local_err = NULL;
- bool ret;
-
- ret = systemd_supports_mode(mode, &local_err);
- if (ret) {
- return;
- }
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
- ret = pmutils_supports_mode(mode, &local_err);
- if (ret) {
- return;
- }
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
- ret = linux_sys_state_supports_mode(mode, &local_err);
- if (!ret) {
- error_setg(errp,
- "the requested suspend mode is not supported by the guest");
- }
-}
-
static void guest_suspend(SuspendMode mode, Error **errp)
{
Error *local_err = NULL;
+ bool mode_supported = false;
- bios_supports_mode(mode, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
+ if (systemd_supports_mode(mode, &local_err)) {
+ mode_supported = true;
+ systemd_suspend(mode, &local_err);
}
- systemd_suspend(mode, &local_err);
if (!local_err) {
return;
}
local_err = NULL;
- pmutils_suspend(mode, &local_err);
+ if (pmutils_supports_mode(mode, &local_err)) {
+ mode_supported = true;
+ pmutils_suspend(mode, &local_err);
+ }
+
if (!local_err) {
return;
}
local_err = NULL;
- linux_sys_state_suspend(mode, &local_err);
- if (local_err) {
+ if (linux_sys_state_supports_mode(mode, &local_err)) {
+ mode_supported = true;
+ linux_sys_state_suspend(mode, &local_err);
+ }
+
+ if (!mode_supported) {
+ error_setg(errp,
+ "the requested suspend mode is not supported by the guest");
+ } else if (local_err) {
error_propagate(errp, local_err);
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep
2018-06-19 19:38 [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep Daniel Henrique Barboza
` (5 preceding siblings ...)
2018-06-19 19:38 ` [Qemu-devel] [PATCH v1 6/6] qga: removing bios_supports_mode Daniel Henrique Barboza
@ 2018-06-19 21:56 ` no-reply
6 siblings, 0 replies; 12+ messages in thread
From: no-reply @ 2018-06-19 21:56 UTC (permalink / raw)
To: danielhb413; +Cc: famz, qemu-devel, armbru, mdroth
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180619193806.17419-1-danielhb413@gmail.com
Subject: [Qemu-devel] [PATCH v1 0/6] QGA: systemd hibernate/suspend/hybrid-sleep
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
cc696f1009 qga: removing bios_supports_mode
dc712bbec2 qga: adding systemd hibernate/suspend/hybrid-sleep support
3be456b7f1 qga: removing switch statements, adding run_process_child
31cf0e6b15 qga: guest_suspend: decoupling pm-utils and sys logic
314d3a05ca qga: bios_supports_mode: decoupling pm-utils and sys logic
da135a7c9f qga: refactoring qmp_guest_suspend_* functions
=== OUTPUT BEGIN ===
Checking PATCH 1/6: qga: refactoring qmp_guest_suspend_* functions...
Checking PATCH 2/6: qga: bios_supports_mode: decoupling pm-utils and sys logic...
Checking PATCH 3/6: qga: guest_suspend: decoupling pm-utils and sys logic...
Checking PATCH 4/6: qga: removing switch statements, adding run_process_child...
ERROR: "(foo* const*)" should be "(foo * const*)"
#91: FILE: qga/commands-posix.c:1467:
+ execve(cmd_path, (char* const*)command, environ);
ERROR: space required before that '*' (ctx:VxB)
#91: FILE: qga/commands-posix.c:1467:
+ execve(cmd_path, (char* const*)command, environ);
^
total: 2 errors, 0 warnings, 295 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 5/6: qga: adding systemd hibernate/suspend/hybrid-sleep support...
Checking PATCH 6/6: qga: removing bios_supports_mode...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 12+ messages in thread