[Qemu-devel] [PULL 00/14] qga patch queue for soft-freeze

[Qemu-devel] [PULL 00/14] qga patch queue for soft-freeze

[Michael Roth]

Hi Peter,

Sorry for the last-minute pull request. Most of these have been on the list
for a while now waiting to be applied. Please consider pulling these in.

The following changes since commit b07cd3e748b3f27a17c27afeee578dc4eedb8dd5:

  Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-3.0-20180703' into staging (2018-07-03 14:59:27 +0100)

are available in the git repository at:

  git://github.com/mdroth/qemu.git tags/qga-pull-2018-07-03-tag

for you to fetch changes up to 73e1d8eb9b738cef3dee2da26bb669b1092a4c12:

  qga: removing bios_supports_mode (2018-07-03 15:20:51 -0500)

----------------------------------------------------------------
qemu-ga patch queue for soft-freeze

* add systemd suspend support
* add used/total space stats for guest-get-fsinfo
* fixes for guest-get-fsinfo over PCI bridges
* MSI installer and schema doc fixes
* guard against unbounded allocations in guest-file-read
* add some additional qga test cases

----------------------------------------------------------------
Bishara AbuHattoum (1):
      qga-win: Fixing msi upgrade disallow in WiX file

Chen Hanxiao (3):
      qga: unset frozen state if no mount points are frozen
      qga: add mountpoint usage info to GuestFilesystemInfo
      qga-win: add driver path usage to GuestFilesystemInfo

Daniel Henrique Barboza (6):
      qga: refactoring qmp_guest_suspend_* functions
      qga: bios_supports_mode: decoupling pm-utils and sys logic
      qga: guest_suspend: decoupling pm-utils and sys logic
      qga: removing switch statements, adding run_process_child
      qga: systemd hibernate/suspend/hybrid-sleep support
      qga: removing bios_supports_mode

Marc-André Lureau (1):
      qemu-ga: make get-fsinfo work over pci bridges

Prasad J Pandit (1):
      qga: check bytes count read by guest-file-read

Tomáš Golembiovský (2):
      test-qga: add trivial tests for some commands
      qga/schema: fix documentation for GuestOSInfo

 qga/commands-posix.c      | 362 +++++++++++++++++++++++++++++++---------------
 qga/commands-win32.c      |  14 +-
 qga/installer/qemu-ga.wxs |   2 +-
 qga/qapi-schema.json      |  11 +-
 tests/test-qga.c          |  54 +++++++
 5 files changed, 325 insertions(+), 118 deletions(-)

[Qemu-devel] [PULL 01/14] qga: unset frozen state if no mount points are frozen

[Michael Roth]

From: Chen Hanxiao <chenhanxiao@gmail.com>

If we set mountpoints to qmp_guest_fsfreeze_freeze_list,
we may got nothing to freeze as all mountpoints are
not valid.
So call ga_unset_frozen in this senario.

Also, if we return 0 frozen fs, there is no need to call
guest-fsfreeze-thaw.

Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 6 ++++++
 qga/qapi-schema.json | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index eae817191b..594d21ef3e 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1274,6 +1274,12 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
     }
 
     free_fs_mount_list(&mounts);
+    /* We may not issue any FIFREEZE here.
+     * Just unset ga_state here and ready for the next call.
+     */
+    if (i == 0) {
+        ga_unset_frozen(ga_state);
+    }
     return i;
 
 error:
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 17884c7c70..1045cef386 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -435,7 +435,9 @@
 # for up to 10 seconds by VSS.
 #
 # Returns: Number of file systems currently frozen. On error, all filesystems
-# will be thawed.
+# will be thawed. If no filesystems are frozen as a result of this call,
+# then @guest-fsfreeze-status will remain "thawed" and calling
+# @guest-fsfreeze-thaw is not necessary.
 #
 # Since: 0.15.0
 ##
-- 
2.11.0

[Qemu-devel] [PULL 02/14] qga: check bytes count read by guest-file-read

[Michael Roth]

From: Prasad J Pandit <pjp@fedoraproject.org>

While reading file content via 'guest-file-read' command,
'qmp_guest_file_read' routine allocates buffer of count+1
bytes. It could overflow for large values of 'count'.
Add check to avoid it.

Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Cc: qemu-stable@nongnu.org
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 2 +-
 qga/commands-win32.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 594d21ef3e..9284e71666 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -458,7 +458,7 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
 
     if (!has_count) {
         count = QGA_READ_COUNT_DEFAULT;
-    } else if (count < 0) {
+    } else if (count < 0 || count >= UINT32_MAX) {
         error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
                    count);
         return NULL;
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 70ee5379f6..73f31fa8c2 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -318,7 +318,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
     }
     if (!has_count) {
         count = QGA_READ_COUNT_DEFAULT;
-    } else if (count < 0) {
+    } else if (count < 0 || count >= UINT32_MAX) {
         error_setg(errp, "value '%" PRId64
                    "' is invalid for argument count", count);
         return NULL;
-- 
2.11.0

[Qemu-devel] [PULL 03/14] qga: add mountpoint usage info to GuestFilesystemInfo

[Michael Roth]

From: Chen Hanxiao <chenhanxiao@gmail.com>

This patch adds support for getting the usage of mounted
filesystem.
The usage of fs stored as used_bytes and total_bytes.
It's very useful when we try to monitor guest's filesystem.

Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 15 +++++++++++++++
 qga/qapi-schema.json |  3 +++
 2 files changed, 18 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 9284e71666..ae8535e497 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -46,6 +46,7 @@ extern char **environ;
 #include <arpa/inet.h>
 #include <sys/socket.h>
 #include <net/if.h>
+#include <sys/statvfs.h>
 
 #ifdef FIFREEZE
 #define CONFIG_FSFREEZE
@@ -1072,6 +1073,8 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
                                                Error **errp)
 {
     GuestFilesystemInfo *fs = g_malloc0(sizeof(*fs));
+    struct statvfs buf;
+    unsigned long used, nonroot_total, fr_size;
     char *devpath = g_strdup_printf("/sys/dev/block/%u:%u",
                                     mount->devmajor, mount->devminor);
 
@@ -1079,7 +1082,19 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
     fs->type = g_strdup(mount->devtype);
     build_guest_fsinfo_for_device(devpath, fs, errp);
 
+    if (statvfs(fs->mountpoint, &buf) == 0) {
+        fr_size = buf.f_frsize;
+        used = buf.f_blocks - buf.f_bfree;
+        nonroot_total = used + buf.f_bavail;
+        fs->used_bytes = used * fr_size;
+        fs->total_bytes = nonroot_total * fr_size;
+
+        fs->has_total_bytes = true;
+        fs->has_used_bytes = true;
+    }
+
     g_free(devpath);
+
     return fs;
 }
 
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 1045cef386..2df6356b8c 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -848,6 +848,8 @@
 # @name: disk name
 # @mountpoint: mount point path
 # @type: file system type string
+# @used-bytes: file system used bytes (since 3.0)
+# @total-bytes: non-root file system total bytes (since 3.0)
 # @disk: an array of disk hardware information that the volume lies on,
 #        which may be empty if the disk type is not supported
 #
@@ -855,6 +857,7 @@
 ##
 { 'struct': 'GuestFilesystemInfo',
   'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
+           '*used-bytes': 'uint64', '*total-bytes': 'uint64',
            'disk': ['GuestDiskAddress']} }
 
 ##
-- 
2.11.0

[Qemu-devel] [PULL 04/14] qga-win: add driver path usage to GuestFilesystemInfo

[Michael Roth]

From: Chen Hanxiao <chenhanxiao@gmail.com>

This patch adds support for getting the usage of
windows driver path.
The usage of fs stored as used_bytes and total_bytes.

Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-win32.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 73f31fa8c2..318d760a74 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -670,6 +670,7 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
     char fs_name[32];
     char vol_info[MAX_PATH+1];
     size_t len;
+    uint64_t i64FreeBytesToCaller, i64TotalBytes, i64FreeBytes;
     GuestFilesystemInfo *fs = NULL;
 
     GetVolumePathNamesForVolumeName(guid, (LPCH)&mnt, 0, &info_size);
@@ -699,10 +700,21 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
     fs_name[sizeof(fs_name) - 1] = 0;
     fs = g_malloc(sizeof(*fs));
     fs->name = g_strdup(guid);
+    fs->has_total_bytes = false;
+    fs->has_used_bytes = false;
     if (len == 0) {
         fs->mountpoint = g_strdup("System Reserved");
     } else {
         fs->mountpoint = g_strndup(mnt_point, len);
+        if (GetDiskFreeSpaceEx(fs->mountpoint,
+                               (PULARGE_INTEGER) & i64FreeBytesToCaller,
+                               (PULARGE_INTEGER) & i64TotalBytes,
+                               (PULARGE_INTEGER) & i64FreeBytes)) {
+            fs->used_bytes = i64TotalBytes - i64FreeBytes;
+            fs->total_bytes = i64TotalBytes;
+            fs->has_total_bytes = true;
+            fs->has_used_bytes = true;
+        }
     }
     fs->type = g_strdup(fs_name);
     fs->disk = build_guest_disk_info(guid, errp);
-- 
2.11.0

[Qemu-devel] [PULL 05/14] test-qga: add trivial tests for some commands

[Michael Roth]

From: Tomáš Golembiovský <tgolembi@redhat.com>

These commands did not get their tests in the original commits:
- guest-get-host-name
- guest-get-timezone
- guest-get-users

Trivial tests that mostly only call the commands were added.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
* replace QDECREF() with qobject_unref()
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 tests/test-qga.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/tests/test-qga.c b/tests/test-qga.c
index 30c9643257..350f27812a 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -854,6 +854,54 @@ static void test_qga_guest_exec_invalid(gconstpointer fix)
     qobject_unref(ret);
 }
 
+static void test_qga_guest_get_host_name(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    QDict *ret, *val;
+
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-host-name'}");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+
+    val = qdict_get_qdict(ret, "return");
+    g_assert(qdict_haskey(val, "host-name"));
+
+    qobject_unref(ret);
+}
+
+static void test_qga_guest_get_timezone(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    QDict *ret, *val;
+
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-timezone'}");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+
+    /* Make sure there's at least offset */
+    val = qdict_get_qdict(ret, "return");
+    g_assert(qdict_haskey(val, "offset"));
+
+    qobject_unref(ret);
+}
+
+static void test_qga_guest_get_users(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    QDict *ret;
+    QList *val;
+
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-users'}");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+
+    /* There is not much to test here */
+    val = qdict_get_qlist(ret, "return");
+    g_assert_nonnull(val);
+
+    qobject_unref(ret);
+}
+
 static void test_qga_guest_get_osinfo(gconstpointer data)
 {
     TestFixture fixture;
@@ -946,6 +994,12 @@ int main(int argc, char **argv)
                          test_qga_guest_exec_invalid);
     g_test_add_data_func("/qga/guest-get-osinfo", &fix,
                          test_qga_guest_get_osinfo);
+    g_test_add_data_func("/qga/guest-get-host-name", &fix,
+                         test_qga_guest_get_host_name);
+    g_test_add_data_func("/qga/guest-get-timezone", &fix,
+                         test_qga_guest_get_timezone);
+    g_test_add_data_func("/qga/guest-get-users", &fix,
+                         test_qga_guest_get_users);
 
     ret = g_test_run();
 
-- 
2.11.0

[Qemu-devel] [PULL 06/14] qga/schema: fix documentation for GuestOSInfo

[Michael Roth]

From: Tomáš Golembiovský <tgolembi@redhat.com>

The documentation for kernel-version and kernel-release on Windows was
swapped.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/qapi-schema.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 2df6356b8c..dfbc4a5e32 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1173,10 +1173,10 @@
 #
 # @kernel-release:
 #     * POSIX: release field returned by uname(2)
-#     * Windows: version number of the OS
+#     * Windows: build number of the OS
 # @kernel-version:
 #     * POSIX: version field returned by uname(2)
-#     * Windows: build number of the OS
+#     * Windows: version number of the OS
 # @machine:
 #     * POSIX: machine field returned by uname(2)
 #     * Windows: one of x86, x86_64, arm, ia64
-- 
2.11.0

[Qemu-devel] [PULL 07/14] qga-win: Fixing msi upgrade disallow in WiX file

[Michael Roth]

From: Bishara AbuHattoum <bishara@daynix.com>

Issue: When upgrading qemu-ga using the msi from an old version
       to a newer one, the upgrade is not allowed by the msi
       showing this error message "Another version of this product
       is already installed."

BZ# 1536331: https://bugzilla.redhat.com/show_bug.cgi?id=1536331

Fix: For the upgrade to be allowed by the msi the WiX file must
     provide three things:
      1. Changing product's Id. (assigning it to "*")
      2. Constant product's UpgradeId. (exists)
      3. Changing version. (exists)

Reference: http://wixtoolset.org/documentation/manual/v3/howtos/updates/major_upgrade.html

Signed-off-by: Bishara AbuHattoum <bishara@daynix.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/installer/qemu-ga.wxs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 5af11627f8..f751a7e9f7 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -41,7 +41,7 @@
 
   <Product
     Name="QEMU guest agent"
-    Id="{DF9974AD-E41A-4304-81AD-69AA8F299766}"
+    Id="*"
     UpgradeCode="{EB6B8302-C06E-4BEC-ADAC-932C68A3A98D}"
     Manufacturer="$(env.QEMU_GA_MANUFACTURER)"
     Version="$(env.QEMU_GA_VERSION)"
-- 
2.11.0

[Qemu-devel] [PULL 08/14] qemu-ga: make get-fsinfo work over pci bridges

[Michael Roth]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Iterate over the PCI bridges to lookup the PCI device associated with
the block device.

This allows to lookup the driver under the following syspath:
/sys/devices/pci0000:00/0000:00:02.2/0000:03:00.0/virtio2/block/vda/vda3

It also works with an "old-style" Q35 libvirt hierarchy: root complex
-> DMI-PCI bridge -> PCI-PCI bridge -> virtio controller, ex:
/sys/devices/pci0000:00/0000:00:03.0/0000:01:01.0/0000:02:01.0/virtio1/block/vda/vda3

The setup can be reproduced with the following qemu command line
(Thanks Marcel for help):

qemu-system-x86_64 -M q35 \
  -device i82801b11-bridge,id=dmi2pci_bridge,bus=pcie.0
  -device pci-bridge,id=pci_bridge,bus=dmi2pci_bridge,addr=0x1,chassis_nr=1
  -device virtio-blk-pci,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,bus=pci_bridge,addr=0x1

For consistency with other syspath-related debug messages, replace a
\"%s\" in the message with '%s'.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1567041

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index ae8535e497..7b7c78d85a 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -876,13 +876,28 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
     p = strstr(syspath, "/devices/pci");
     if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n",
                      pci, pci + 1, pci + 2, pci + 3, &pcilen) < 4) {
-        g_debug("only pci device is supported: sysfs path \"%s\"", syspath);
+        g_debug("only pci device is supported: sysfs path '%s'", syspath);
         return;
     }
 
-    driver = get_pci_driver(syspath, (p + 12 + pcilen) - syspath, errp);
-    if (!driver) {
-        goto cleanup;
+    p += 12 + pcilen;
+    while (true) {
+        driver = get_pci_driver(syspath, p - syspath, errp);
+        if (driver && (g_str_equal(driver, "ata_piix") ||
+                       g_str_equal(driver, "sym53c8xx") ||
+                       g_str_equal(driver, "virtio-pci") ||
+                       g_str_equal(driver, "ahci"))) {
+            break;
+        }
+
+        if (sscanf(p, "/%x:%x:%x.%x%n",
+                          pci, pci + 1, pci + 2, pci + 3, &pcilen) == 4) {
+            p += pcilen;
+            continue;
+        }
+
+        g_debug("unsupported driver or sysfs path '%s'", syspath);
+        return;
     }
 
     p = strstr(syspath, "/target");
-- 
2.11.0

[Qemu-devel] [PULL 09/14] qga: refactoring qmp_guest_suspend_* functions

[Michael Roth]

From: Daniel Henrique Barboza <danielhb413@gmail.com>

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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.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 7b7c78d85a..bd2c3514d8 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1474,15 +1474,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();
@@ -1559,14 +1582,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();
@@ -1629,42 +1677,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.11.0

[Qemu-devel] [PULL 10/14] qga: bios_supports_mode: decoupling pm-utils and sys logic

[Michael Roth]

From: Daniel Henrique Barboza <danielhb413@gmail.com>

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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.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 bd2c3514d8..a8299ecc77 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1478,75 +1478,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");
@@ -1559,17 +1527,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,
@@ -1580,6 +1542,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.11.0

[Qemu-devel] [PULL 11/14] qga: guest_suspend: decoupling pm-utils and sys logic

[Michael Roth]

From: Daniel Henrique Barboza <danielhb413@gmail.com>

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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.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 a8299ecc77..9ebd77b894 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1545,6 +1545,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;
@@ -1581,64 +1640,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;
 
@@ -1647,19 +1670,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);
@@ -1672,27 +1682,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;
+    }
+
+    error_free(local_err);
+
+    linux_sys_state_suspend(suspend_mode, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
 }
 
 void qmp_guest_suspend_disk(Error **errp)
-- 
2.11.0

[Qemu-devel] [PULL 12/14] qga: removing switch statements, adding run_process_child

[Michael Roth]

From: Daniel Henrique Barboza <danielhb413@gmail.com>

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 uses g_spawn_sync() to execute a shell command,
returning the exit 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>
*check/propagate local_err before setting errp directly
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 215 +++++++++++++++++++++------------------------------
 1 file changed, 88 insertions(+), 127 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 9ebd77b894..1559753d85 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1474,152 +1474,117 @@ 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;
+
+/*
+ * Executes a command in a child process using g_spawn_sync,
+ * returning an int >= 0 representing the exit status of the
+ * process.
+ *
+ * If the program wasn't found in path, returns -1.
+ *
+ * If a problem happened when creating the child process,
+ * returns -1 and errp is set.
+ */
+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;
-    pid_t pid;
-    int status;
-    bool ret = false;
+    int exit_status, spawn_flag;
+    GError *g_err = NULL;
+    bool success;
 
-    switch (suspend_mode) {
+    spawn_flag = G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL |
+                 G_SPAWN_STDERR_TO_DEV_NULL;
 
-    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;
+    success =  g_spawn_sync(NULL, (char **)command, environ, spawn_flag,
+                            NULL, NULL, NULL, NULL,
+                            &exit_status, &g_err);
+
+    if (success) {
+        return WEXITSTATUS(exit_status);
     }
 
-    pmutils_path = g_find_program_in_path(pmutils_bin);
-    if (!pmutils_path) {
-        return ret;
+    if (g_err && (g_err->code != G_SPAWN_ERROR_NOENT)) {
+        error_setg(errp, "failed to create child process, error '%s'",
+                   g_err->message);
     }
 
-    pid = fork();
-    if (!pid) {
-        setsid();
-        execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
-        /*
-         * If we get here execle() has failed.
-         */
-        _exit(SUSPEND_NOT_SUPPORTED);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        goto out;
+    g_error_free(g_err);
+    return -1;
+}
+
+static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
+{
+    Error *local_err = NULL;
+    const char *pmutils_args[3] = {"--hibernate", "--suspend",
+                                   "--suspend-hybrid"};
+    const char *cmd[3] = {"pm-is-supported", pmutils_args[mode], NULL};
+    int status;
+
+    status = run_process_child(cmd, &local_err);
+
+    if (status == SUSPEND_SUPPORTED) {
+        return true;
     }
 
-    ga_wait_child(pid, &status, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto out;
+    if ((status == -1) && !local_err) {
+        return false;
     }
 
-    switch (WEXITSTATUS(status)) {
-    case SUSPEND_SUPPORTED:
-        ret = true;
-        goto out;
-    case SUSPEND_NOT_SUPPORTED:
-        goto out;
-    default:
+    if (local_err) {
+        error_propagate(errp, local_err);
+    } else {
         error_setg(errp,
-                   "the helper program '%s' returned an unexpected exit status"
-                   " code (%d)", pmutils_path, WEXITSTATUS(status));
-        goto out;
+                   "the helper program '%s' returned an unexpected exit"
+                   " status code (%d)", "pm-is-supported", status);
     }
 
-out:
-    g_free(pmutils_path);
-    return ret;
+    return false;
 }
 
-static void pmutils_suspend(int suspend_mode, Error **errp)
+static void pmutils_suspend(SuspendMode mode, Error **errp)
 {
     Error *local_err = NULL;
-    const char *pmutils_bin;
-    char *pmutils_path;
-    pid_t pid;
+    const char *pmutils_binaries[3] = {"pm-hibernate", "pm-suspend",
+                                       "pm-suspend-hybrid"};
+    const char *cmd[2] = {pmutils_binaries[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);
+    if (status == 0) {
         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;
+    if ((status == -1) && !local_err) {
+        error_setg(errp, "the helper program '%s' was not found",
+                   pmutils_binaries[mode]);
+        return;
     }
 
-    ga_wait_child(pid, &status, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        goto out;
-    }
-
-    if (WEXITSTATUS(status)) {
+    } else {
         error_setg(errp,
-                   "the helper program '%s' returned an unexpected exit status"
-                   " code (%d)", pmutils_path, WEXITSTATUS(status));
+                   "the helper program '%s' returned an unexpected exit"
+                   " status code (%d)", pmutils_binaries[mode], status);
     }
-
-out:
-    g_free(pmutils_path);
 }
 
-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;
     }
 
@@ -1640,22 +1605,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;
     }
@@ -1697,12 +1655,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;
     }
@@ -1710,32 +1668,35 @@ 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 (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     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;
     }
 
     error_free(local_err);
 
-    linux_sys_state_suspend(suspend_mode, &local_err);
+    linux_sys_state_suspend(mode, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
     }
-- 
2.11.0

[Qemu-devel] [PULL 13/14] qga: systemd hibernate/suspend/hybrid-sleep support

[Michael Roth]

From: Daniel Henrique Barboza <danielhb413@gmail.com>

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
types 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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1559753d85..cdb825993f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1517,6 +1517,63 @@ static int run_process_child(const char *command[], Error **errp)
     return -1;
 }
 
+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) && !local_err) {
+        error_setg(errp, "the helper program 'systemctl %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;
@@ -1660,6 +1717,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;
@@ -1689,6 +1754,13 @@ static void guest_suspend(SuspendMode mode, Error **errp)
         return;
     }
 
+    systemd_suspend(mode, &local_err);
+    if (!local_err) {
+        return;
+    }
+
+    error_free(local_err);
+
     pmutils_suspend(mode, &local_err);
     if (!local_err) {
         return;
-- 
2.11.0

[Qemu-devel] [PULL 14/14] qga: removing bios_supports_mode

[Michael Roth]

From: Daniel Henrique Barboza <danielhb413@gmail.com>

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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 58 ++++++++++++++++------------------------------------
 1 file changed, 18 insertions(+), 40 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index cdb825993f..233f78a406 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1712,64 +1712,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 (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-    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;
     }
 
     error_free(local_err);
 
-    pmutils_suspend(mode, &local_err);
+    if (pmutils_supports_mode(mode, &local_err)) {
+        mode_supported = true;
+        pmutils_suspend(mode, &local_err);
+    }
+
     if (!local_err) {
         return;
     }
 
     error_free(local_err);
 
-    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.11.0

Re: [Qemu-devel] [PULL 00/14] qga patch queue for soft-freeze

[Peter Maydell]

On 3 July 2018 at 21:52, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Hi Peter,
>
> Sorry for the last-minute pull request. Most of these have been on the list
> for a while now waiting to be applied. Please consider pulling these in.
>
> The following changes since commit b07cd3e748b3f27a17c27afeee578dc4eedb8dd5:
>
>   Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-3.0-20180703' into staging (2018-07-03 14:59:27 +0100)
>
> are available in the git repository at:
>
>   git://github.com/mdroth/qemu.git tags/qga-pull-2018-07-03-tag
>
> for you to fetch changes up to 73e1d8eb9b738cef3dee2da26bb669b1092a4c12:
>
>   qga: removing bios_supports_mode (2018-07-03 15:20:51 -0500)
>
> ----------------------------------------------------------------
> qemu-ga patch queue for soft-freeze
>
> * add systemd suspend support
> * add used/total space stats for guest-get-fsinfo
> * fixes for guest-get-fsinfo over PCI bridges
> * MSI installer and schema doc fixes
> * guard against unbounded allocations in guest-file-read
> * add some additional qga test cases
>
> ----------------------------------------------------------------
Applied, thanks.

-- PMM