[Qemu-devel] [PATCH v7 0/3] qemu-ga: add guest-get-osinfo command

[Qemu-devel] [PATCH v7 0/3] qemu-ga: add guest-get-osinfo command

[Tomáš Golembiovský]

v7:
- fixed incorrect error check in ga_get_win_version()
- added missing test-qga-os-release
- fixed coding style issues

v6:
- fixed the documentation comment in schema
- disguising os-release as key-value file (thanks Marc-André)
- dropped dependency on gio
- improved error handling
- added test

v5:
- fixed build failure with older glib
- fixed coding style issues
- fixed one log string

This is a continuation of the work started by Vinzenz Feenstra in the
threads:

https://lists.nongnu.org/archive/html/qemu-devel/2017-03/msg04154.html
https://lists.nongnu.org/archive/html/qemu-devel/2017-03/msg04302.html
https://lists.nongnu.org/archive/html/qemu-devel/2017-03/msg06262.html

The idea is to report some basic information from uname and from
os-release file, if it is present. On MS Windows, where neither uname
nor os-release exist we fill the values based on the information we can
get from the OS.

The example output on Fedora is:

{
  "return": {
    "kernel-version": "#1 SMP Mon May 8 18:46:06 UTC 2017",
    "kernel-release": "4.10.15-200.fc25.x86_64",
    "machine-hardware": "x86_64",
    "id": "fedora",
    "name": "Fedora",
    "pretty-name": "Fedora 25 (Server Edition)",
    "version": "25 (Server Edition)",
    "variant": "Server Edition",
    "version-id": "25",
    "variant-id": "server"
  }
}

The example output on MS Windows 10 is:

{
  "return": {
    "kernel-version": "10.0",
    "kernel-release": "10240",
    "machine-hardware": "x86_64",
    "id": "mswindows",
    "name": "Microsoft Windows",
    "pretty-name": "Windows 10 Enterprise",
    "version": "Microsoft Windows 10",
    "version-id": "10",
    "variant": "client",
    "variant-id": "client"
  }
}

    Tomas Golembiovsky

Tomáš Golembiovský (3):
  qemu-ga: add guest-get-osinfo command
  test-qga: pass environemnt to qemu-ga
  test-qga: add test for guest-get-osinfo

 qga/commands-posix.c           | 143 +++++++++++++++++++++++++++++++
 qga/commands-win32.c           | 185 +++++++++++++++++++++++++++++++++++++++++
 qga/qapi-schema.json           |  65 +++++++++++++++
 tests/data/test-qga-os-release |   7 ++
 tests/test-qga.c               |  63 +++++++++++++-
 5 files changed, 459 insertions(+), 4 deletions(-)
 create mode 100644 tests/data/test-qga-os-release

-- 
2.13.1

[Qemu-devel] [PATCH v7 1/3] qemu-ga: add guest-get-osinfo command

[Tomáš Golembiovský]

Add a new 'guest-get-osinfo' command for reporting basic information of
the guest operating system. This includes machine architecture,
version and release of the kernel and several fields from os-release
file if it is present (as defined in [1]).

[1] https://www.freedesktop.org/software/systemd/man/os-release.html

Signed-off-by: Vinzenz Feenstra <vfeenstr@redhat.com>
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 qga/commands-posix.c | 136 +++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/qapi-schema.json |  65 ++++++++++++++++++
 3 files changed, 386 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index d8e412275e..7a933a5f12 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include <sys/ioctl.h>
+#include <sys/utsname.h>
 #include <sys/wait.h>
 #include <dirent.h>
 #include <utmpx.h>
@@ -2577,3 +2578,138 @@ GuestUserList *qmp_guest_get_users(Error **err)
     g_hash_table_destroy(cache);
     return head;
 }
+
+/* Replace escaped special characters with theire real values. The replacement
+ * is done in place -- returned value is in the original string.
+ */
+static void ga_osrelease_replace_special(gchar *value)
+{
+    gchar *p, *p2, quote;
+
+    /* Trim the string at first space or semicolon if it is not enclosed in
+     * single or double quotes. */
+    if ((value[0] != '"') || (value[0] == '\'')) {
+        p = strchr(value, ' ');
+        if (p != NULL) {
+            *p = 0;
+        }
+        p = strchr(value, ';');
+        if (p != NULL) {
+            *p = 0;
+        }
+        return;
+    }
+
+    quote = value[0];
+    p2 = value;
+    p = value + 1;
+    while (*p != 0) {
+        if (*p == '\\') {
+            p++;
+            switch (*p) {
+            case '$':
+            case '\'':
+            case '"':
+            case '\\':
+            case '`':
+                break;
+            default:
+                /* Keep literal backslash followed by whatever is there */
+                p--;
+                break;
+            }
+        } else if (*p == quote) {
+            *p2 = 0;
+            break;
+        }
+        *(p2++) = *(p++);
+    }
+}
+
+static GKeyFile *ga_parse_osrelease(const char *fname)
+{
+    gboolean ret;
+    gchar *content = NULL;
+    gchar *content2 = NULL;
+    gsize len;
+    GError *err = NULL;
+    GKeyFile *keys = g_key_file_new();
+    const char *group = "[os-release]\n";
+
+    ret = g_file_get_contents(fname, &content, &len, &err);
+    if (ret != TRUE) {
+        slog("failed to read '%s', error: %s", fname, err->message);
+        goto fail;
+    }
+
+    ret = g_utf8_validate(content, len, NULL);
+    if (ret != TRUE) {
+        slog("file is not utf-8 encoded: %s", fname);
+        goto fail;
+    }
+    content2 = g_strdup_printf("%s%s", group, content);
+    len += strlen(group);
+
+    ret = g_key_file_load_from_data(keys, content2, len, G_KEY_FILE_NONE,
+        &err);
+    if (ret != TRUE) {
+        slog("failed to parse file '%s', error: %s", fname, err->message);
+        goto fail;
+    }
+
+    g_free(content);
+    g_free(content2);
+    return keys;
+
+fail:
+    g_error_free(err);
+    g_free(content);
+    g_free(content2);
+    g_key_file_free(keys);
+    return NULL;
+}
+
+GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
+{
+    GuestOSInfo *info = g_new0(GuestOSInfo, 1);
+
+    struct utsname kinfo = {0};
+    if (uname(&kinfo) != 0) {
+        error_setg_errno(errp, errno, "uname failed");
+        return NULL;
+    }
+
+    info->kernel_version = g_strdup(kinfo.version);
+    info->kernel_release = g_strdup(kinfo.release);
+    info->machine_hardware = g_strdup(kinfo.machine);
+
+    GKeyFile *osrelease = ga_parse_osrelease("/etc/os-release");
+    if (osrelease == NULL) {
+        osrelease = ga_parse_osrelease("/usr/lib/os-release");
+    }
+
+    if (osrelease != NULL) {
+        char *value;
+
+#define GET_FIELD(field, osfield) do { \
+    value = g_key_file_get_value(osrelease, "os-release", osfield, NULL); \
+    if (value != NULL) { \
+        ga_osrelease_replace_special(value); \
+        info->has_ ## field = true; \
+        info->field = value; \
+    } \
+} while (0)
+        GET_FIELD(id, "ID");
+        GET_FIELD(name, "NAME");
+        GET_FIELD(pretty_name, "PRETTY_NAME");
+        GET_FIELD(version, "VERSION");
+        GET_FIELD(version_id, "VERSION_ID");
+        GET_FIELD(variant, "VARIANT");
+        GET_FIELD(variant_id, "VARIANT_ID");
+#undef GET_FIELD
+
+        g_key_file_free(osrelease);
+    }
+
+    return info;
+}
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 439d229225..e471f5561b 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1639,3 +1639,188 @@ GuestUserList *qmp_guest_get_users(Error **err)
     return NULL;
 #endif
 }
+
+typedef struct _ga_matrix_lookup_t {
+    int major;
+    int minor;
+    char const *version;
+    char const *version_id;
+} ga_matrix_lookup_t;
+
+static ga_matrix_lookup_t const WIN_VERSION_MATRIX[2][8] = {
+    {
+        /* Desktop editions */
+        { 5, 0, "Microsoft Windows 2000",   "2000"},
+        { 5, 1, "Microsoft Windows XP",     "xp"},
+        { 6, 0, "Microsoft Windows Vista",  "vista"},
+        { 6, 1, "Microsoft Windows 7"       "7"},
+        { 6, 2, "Microsoft Windows 8",      "8"},
+        { 6, 3, "Microsoft Windows 8.1",    "8.1"},
+        {10, 0, "Microsoft Windows 10",     "10"},
+        { 0, 0, 0}
+    },{
+        /* Server editions */
+        { 5, 2, "Microsoft Windows Server 2003",        "2003"},
+        { 6, 0, "Microsoft Windows Server 2008",        "2008"},
+        { 6, 1, "Microsoft Windows Server 2008 R2",     "2008r2"},
+        { 6, 2, "Microsoft Windows Server 2012",        "2012"},
+        { 6, 3, "Microsoft Windows Server 2012 R2",     "2012r2"},
+        {10, 0, "Microsoft Windows Server 2016",        "2016"},
+        { 0, 0, 0},
+        { 0, 0, 0}
+    }
+};
+
+static void ga_get_win_version(RTL_OSVERSIONINFOEXW *info, Error **errp)
+{
+    typedef NTSTATUS(WINAPI * rtl_get_version_t)(
+        RTL_OSVERSIONINFOEXW *os_version_info_ex);
+
+    info->dwOSVersionInfoSize = sizeof(RTL_OSVERSIONINFOEXW);
+
+    HMODULE module = GetModuleHandle("ntdll");
+    PVOID fun = GetProcAddress(module, "RtlGetVersion");
+    if (fun == NULL) {
+        error_setg(errp, QERR_QGA_COMMAND_FAILED,
+            "Failed to get address of RtlGetVersion");
+        return;
+    }
+
+    rtl_get_version_t rtl_get_version = (rtl_get_version_t)fun;
+    rtl_get_version(info);
+    return;
+}
+
+static char *ga_get_win_name(OSVERSIONINFOEXW const *os_version, bool id)
+{
+    DWORD major = os_version->dwMajorVersion;
+    DWORD minor = os_version->dwMinorVersion;
+    int tbl_idx = (os_version->wProductType != VER_NT_WORKSTATION);
+    ga_matrix_lookup_t const *table = WIN_VERSION_MATRIX[tbl_idx];
+    while (table->version != NULL) {
+        if (major == table->major && minor == table->minor) {
+            if (id) {
+                return g_strdup(table->version_id);
+            } else {
+                return g_strdup(table->version);
+            }
+        }
+        ++table;
+    }
+    slog("failed to lookup Windows version: major=%lu, minor=%lu",
+        major, minor);
+    return g_strdup("N/A");
+}
+
+static char *ga_get_win_product_name(Error **errp)
+{
+    HKEY key = NULL;
+    DWORD size = 128;
+    char *result = g_malloc0(size);
+    LONG err = ERROR_SUCCESS;
+
+    err = RegOpenKeyA(HKEY_LOCAL_MACHINE,
+                      "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion",
+                      &key);
+    if (err != ERROR_SUCCESS) {
+        error_setg_win32(errp, err, "failed to open registry key");
+        goto fail;
+    }
+
+    err = RegQueryValueExA(key, "ProductName", NULL, NULL,
+                            (LPBYTE)result, &size);
+    if (err == ERROR_MORE_DATA) {
+        slog("ProductName longer than expected (%lu bytes), retrying",
+                size);
+        g_free(result);
+        result = NULL;
+        if (size > 0) {
+            result = g_malloc0(size);
+            err = RegQueryValueExA(key, "ProductName", NULL, NULL,
+                                    (LPBYTE)result, &size);
+        }
+    }
+    if (err != ERROR_SUCCESS) {
+        error_setg_win32(errp, err, "failed to retrive ProductName");
+        goto fail;
+    }
+
+    return result;
+
+fail:
+    g_free(result);
+    return NULL;
+}
+
+static char *ga_get_current_arch(void)
+{
+    SYSTEM_INFO info;
+    GetNativeSystemInfo(&info);
+    char *result = NULL;
+    switch (info.wProcessorArchitecture) {
+    case PROCESSOR_ARCHITECTURE_AMD64:
+        result = g_strdup("x86_64");
+        break;
+    case PROCESSOR_ARCHITECTURE_ARM:
+        result = g_strdup("arm");
+        break;
+    case PROCESSOR_ARCHITECTURE_IA64:
+        result = g_strdup("ia64");
+        break;
+    case PROCESSOR_ARCHITECTURE_INTEL:
+        result = g_strdup("x86");
+        break;
+    case PROCESSOR_ARCHITECTURE_UNKNOWN:
+    default:
+        slog("unknown processor architecture 0x%0x",
+            info.wProcessorArchitecture);
+        result = g_strdup("unknown");
+        break;
+    }
+    return result;
+}
+
+GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
+{
+    Error *local_err = NULL;
+    OSVERSIONINFOEXW os_version = {0};
+
+    ga_get_win_version(&os_version, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
+    bool server = os_version.wProductType != VER_NT_WORKSTATION;
+    char *product_name = ga_get_win_product_name(&local_err);
+    if (product_name == NULL) {
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
+    GuestOSInfo *info = g_new0(GuestOSInfo, 1);
+
+    info->kernel_version = g_strdup_printf("%lu.%lu",
+        os_version.dwMajorVersion,
+        os_version.dwMinorVersion);
+    info->kernel_release = g_strdup_printf("%lu",
+        os_version.dwBuildNumber);
+    info->machine_hardware = ga_get_current_arch();
+
+    info->has_id = true;
+    info->id = g_strdup("mswindows");
+    info->has_name = true;
+    info->name = g_strdup("Microsoft Windows");
+    info->has_pretty_name = true;
+    info->pretty_name = product_name;
+    info->has_version = true;
+    info->version = ga_get_win_name(&os_version, false);
+    info->has_version_id = true;
+    info->version_id = ga_get_win_name(&os_version, true);
+    info->has_variant = true;
+    info->variant = g_strdup(server ? "server" : "client");
+    info->has_variant_id = true;
+    info->variant_id = g_strdup(server ? "server" : "client");
+
+    return info;
+}
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 03743ab905..3d9478c10b 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1126,3 +1126,68 @@
 ##
 { 'command': 'guest-get-timezone',
   'returns': 'GuestTimezone' }
+
+##
+# @GuestOSInfo:
+#
+# @kernel-release:
+#     * _POSIX:_ release field returned by uname(2)
+#     * _Windows:_ version number of the OS
+# @kernel-version:
+#     * _POSIX:_ version field returned by uname(2)
+#     * _Windows:_ build number of the OS
+# @machine-hardware:
+#     * _POSIX:_ machine field returned by uname(2)
+#     * _Windows:_ architecture of the hardware
+# @id:
+#     * _POSIX:_ as defined by os-release(5)
+#     * _Windows:_ contains string "mswindows"
+# @name:
+#     * _POSIX:_ as defined by os-release(5)
+#     * _Windows:_ contains string "Microsoft Windows"
+# @pretty-name:
+#     * _POSIX:_ as defined by os-release(5)
+#     * _Windows:_ product name, e.g. "Microsoft Windows 10 Enterprise"
+# @version:
+#     * _POSIX:_ as defined by os-release(5)
+#     * _Windows:_ long version string, e.g. "Microsoft Windows Server 2008"
+# @version-id:
+#     * _POSIX:_ as defined by os-release(5)
+#     * _Windows:_ short version identifier, e.g. "7" or "20012r2"
+# @variant:
+#     * _POSIX:_ as defined by os-release(5)
+#     * _Windows:_ contains string "server" or "client"
+# @variant-id:
+#     * _POSIX:_ as defined by os-release(5)
+#     * _Windows:_ contains string "server" or "client"
+#
+# Notes:
+#
+# On POSIX systems the fields @id, @name, @pretty-name, @version, @version-id,
+# @variant and @variant-id follow the definition specified in os-release(5).
+# Refer to the manual page for exact description of the fields. Their values
+# are taken from the os-release file. If the file is not present in the system,
+# or the values are not present in the file, the fields are not included.
+#
+# On Windows the values are filled from information gathered from the system.
+#
+# Since: 2.10
+##
+{ 'struct': 'GuestOSInfo',
+  'data': {
+      'kernel-release': 'str', 'kernel-version': 'str',
+      'machine-hardware': 'str', '*id': 'str', '*name': 'str',
+      '*pretty-name': 'str', '*version': 'str', '*version-id': 'str',
+      '*variant': 'str', '*variant-id': 'str' } }
+
+##
+# @guest-get-osinfo:
+#
+# Retrieve guest operating system information
+#
+# Returns: @GuestOSInfo
+#
+# Since: 2.10
+##
+{ 'command': 'guest-get-osinfo',
+  'returns': 'GuestOSInfo' }
-- 
2.13.1

[Qemu-devel] [PATCH v7 2/3] test-qga: pass environemnt to qemu-ga

[Tomáš Golembiovský]

Modify fixture_setup() to pass environemnt variables to spawned qemu-ga
instance.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 tests/test-qga.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/test-qga.c b/tests/test-qga.c
index c77f241036..631b98639a 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -46,7 +46,7 @@ static void qga_watch(GPid pid, gint status, gpointer user_data)
 }
 
 static void
-fixture_setup(TestFixture *fixture, gconstpointer data)
+fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
 {
     const gchar *extra_arg = data;
     GError *error = NULL;
@@ -67,7 +67,7 @@ fixture_setup(TestFixture *fixture, gconstpointer data)
     g_shell_parse_argv(cmd, NULL, &argv, &error);
     g_assert_no_error(error);
 
-    g_spawn_async(fixture->test_dir, argv, NULL,
+    g_spawn_async(fixture->test_dir, argv, envp,
                   G_SPAWN_SEARCH_PATH|G_SPAWN_DO_NOT_REAP_CHILD,
                   NULL, NULL, &fixture->pid, &error);
     g_assert_no_error(error);
@@ -707,7 +707,7 @@ static void test_qga_blacklist(gconstpointer data)
     QDict *ret, *error;
     const gchar *class, *desc;
 
-    fixture_setup(&fix, "-b guest-ping,guest-get-time");
+    fixture_setup(&fix, "-b guest-ping,guest-get-time", NULL);
 
     /* check blacklist */
     ret = qmp_fd(fix.fd, "{'execute': 'guest-ping'}");
@@ -943,7 +943,7 @@ int main(int argc, char **argv)
 
     setlocale (LC_ALL, "");
     g_test_init(&argc, &argv, NULL);
-    fixture_setup(&fix, NULL);
+    fixture_setup(&fix, NULL, NULL);
 
     g_test_add_data_func("/qga/sync-delimited", &fix, test_qga_sync_delimited);
     g_test_add_data_func("/qga/sync", &fix, test_qga_sync);
-- 
2.13.1

[Qemu-devel] [PATCH v7 3/3] test-qga: add test for guest-get-osinfo

[Tomáš Golembiovský]

Add test for guest-get-osinfo command.

Qemu-ga was modified to accept QGA_OS_RELEASE environment variable. If
the variable is defined it is interpreted as path to the os-release file
and it is parsed instead of the default paths.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 qga/commands-posix.c           | 13 +++++++---
 tests/data/test-qga-os-release |  7 ++++++
 tests/test-qga.c               | 55 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 3 deletions(-)
 create mode 100644 tests/data/test-qga-os-release

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 7a933a5f12..16b18c96ef 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2683,9 +2683,16 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
     info->kernel_release = g_strdup(kinfo.release);
     info->machine_hardware = g_strdup(kinfo.machine);
 
-    GKeyFile *osrelease = ga_parse_osrelease("/etc/os-release");
-    if (osrelease == NULL) {
-        osrelease = ga_parse_osrelease("/usr/lib/os-release");
+    GKeyFile *osrelease = NULL;
+
+    const char *qga_os_release = g_getenv("QGA_OS_RELEASE");
+    if (qga_os_release != NULL) {
+        osrelease = ga_parse_osrelease(qga_os_release);
+    } else {
+        osrelease = ga_parse_osrelease("/etc/os-release");
+        if (osrelease == NULL) {
+            osrelease = ga_parse_osrelease("/usr/lib/os-release");
+        }
     }
 
     if (osrelease != NULL) {
diff --git a/tests/data/test-qga-os-release b/tests/data/test-qga-os-release
new file mode 100644
index 0000000000..70664eb6ec
--- /dev/null
+++ b/tests/data/test-qga-os-release
@@ -0,0 +1,7 @@
+ID=qemu-ga-test
+NAME=QEMU-GA
+PRETTY_NAME="QEMU Guest Agent test"
+VERSION="Test 1"
+VERSION_ID=1
+VARIANT="Unit test \"\'\$\`\\ and \\\\ etc."
+VARIANT_ID=unit-test
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 631b98639a..c151dfdc34 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -936,6 +936,59 @@ static void test_qga_guest_exec_invalid(gconstpointer fix)
     QDECREF(ret);
 }
 
+static void test_qga_guest_get_osinfo(gconstpointer data)
+{
+    TestFixture fixture;
+    const gchar *str;
+    gchar *cwd, *env[2];
+    QDict *ret, *val;
+
+    cwd = g_get_current_dir();
+    env[0] = g_strdup_printf(
+        "QGA_OS_RELEASE=%s%ctests%cdata%ctest-qga-os-release",
+        cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
+    env[1] = NULL;
+    g_free(cwd);
+    fixture_setup(&fixture, NULL, env);
+
+    ret = qmp_fd(fixture.fd, "{'execute': 'guest-get-osinfo'}");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+
+    val = qdict_get_qdict(ret, "return");
+
+    str = qdict_get_try_str(val, "id");
+    g_assert_nonnull(str);
+    g_assert_cmpstr(str, ==, "qemu-ga-test");
+
+    str = qdict_get_try_str(val, "name");
+    g_assert_nonnull(str);
+    g_assert_cmpstr(str, ==, "QEMU-GA");
+
+    str = qdict_get_try_str(val, "pretty-name");
+    g_assert_nonnull(str);
+    g_assert_cmpstr(str, ==, "QEMU Guest Agent test");
+
+    str = qdict_get_try_str(val, "version");
+    g_assert_nonnull(str);
+    g_assert_cmpstr(str, ==, "Test 1");
+
+    str = qdict_get_try_str(val, "version-id");
+    g_assert_nonnull(str);
+    g_assert_cmpstr(str, ==, "1");
+
+    str = qdict_get_try_str(val, "variant");
+    g_assert_nonnull(str);
+    g_assert_cmpstr(str, ==, "Unit test \"'$`\\ and \\\\ etc.");
+
+    str = qdict_get_try_str(val, "variant-id");
+    g_assert_nonnull(str);
+    g_assert_cmpstr(str, ==, "unit-test");
+
+    QDECREF(ret);
+    g_free(env[0]);
+}
+
 int main(int argc, char **argv)
 {
     TestFixture fix;
@@ -972,6 +1025,8 @@ int main(int argc, char **argv)
     g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
     g_test_add_data_func("/qga/guest-exec-invalid", &fix,
                          test_qga_guest_exec_invalid);
+    g_test_add_data_func("/qga/guest-get-osinfo", &fix,
+                         test_qga_guest_get_osinfo);
 
     if (g_getenv("QGA_TEST_SIDE_EFFECTING")) {
         g_test_add_data_func("/qga/fsfreeze-and-thaw", &fix,
-- 
2.13.1

Re: [Qemu-devel] [PATCH v7 2/3] test-qga: pass environemnt to qemu-ga

[Marc-André Lureau]

On Mon, Jul 3, 2017 at 1:40 PM Tomáš Golembiovský <tgolembi@redhat.com>
wrote:

> Modify fixture_setup() to pass environemnt variables to spawned qemu-ga
> instance.
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
>

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


---
>  tests/test-qga.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index c77f241036..631b98639a 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -46,7 +46,7 @@ static void qga_watch(GPid pid, gint status, gpointer
> user_data)
>  }
>
>  static void
> -fixture_setup(TestFixture *fixture, gconstpointer data)
> +fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
>  {
>      const gchar *extra_arg = data;
>      GError *error = NULL;
> @@ -67,7 +67,7 @@ fixture_setup(TestFixture *fixture, gconstpointer data)
>      g_shell_parse_argv(cmd, NULL, &argv, &error);
>      g_assert_no_error(error);
>
> -    g_spawn_async(fixture->test_dir, argv, NULL,
> +    g_spawn_async(fixture->test_dir, argv, envp,
>                    G_SPAWN_SEARCH_PATH|G_SPAWN_DO_NOT_REAP_CHILD,
>                    NULL, NULL, &fixture->pid, &error);
>      g_assert_no_error(error);
> @@ -707,7 +707,7 @@ static void test_qga_blacklist(gconstpointer data)
>      QDict *ret, *error;
>      const gchar *class, *desc;
>
> -    fixture_setup(&fix, "-b guest-ping,guest-get-time");
> +    fixture_setup(&fix, "-b guest-ping,guest-get-time", NULL);
>
>      /* check blacklist */
>      ret = qmp_fd(fix.fd, "{'execute': 'guest-ping'}");
> @@ -943,7 +943,7 @@ int main(int argc, char **argv)
>
>      setlocale (LC_ALL, "");
>      g_test_init(&argc, &argv, NULL);
> -    fixture_setup(&fix, NULL);
> +    fixture_setup(&fix, NULL, NULL);
>
>      g_test_add_data_func("/qga/sync-delimited", &fix,
> test_qga_sync_delimited);
>      g_test_add_data_func("/qga/sync", &fix, test_qga_sync);
> --
> 2.13.1
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH v7 1/3] qemu-ga: add guest-get-osinfo command

[Marc-André Lureau]

On Mon, Jul 3, 2017 at 1:40 PM Tomáš Golembiovský <tgolembi@redhat.com>
wrote:

> Add a new 'guest-get-osinfo' command for reporting basic information of
> the guest operating system. This includes machine architecture,
> version and release of the kernel and several fields from os-release
> file if it is present (as defined in [1]).
>
> [1] https://www.freedesktop.org/software/systemd/man/os-release.html
>
> Signed-off-by: Vinzenz Feenstra <vfeenstr@redhat.com>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-posix.c | 136 +++++++++++++++++++++++++++++++++++++
>  qga/commands-win32.c | 185
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/qapi-schema.json |  65 ++++++++++++++++++
>  3 files changed, 386 insertions(+)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index d8e412275e..7a933a5f12 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -13,6 +13,7 @@
>
>  #include "qemu/osdep.h"
>  #include <sys/ioctl.h>
> +#include <sys/utsname.h>
>  #include <sys/wait.h>
>  #include <dirent.h>
>  #include <utmpx.h>
> @@ -2577,3 +2578,138 @@ GuestUserList *qmp_guest_get_users(Error **err)
>      g_hash_table_destroy(cache);
>      return head;
>  }
> +
> +/* Replace escaped special characters with theire real values. The
> replacement
> + * is done in place -- returned value is in the original string.
> + */
> +static void ga_osrelease_replace_special(gchar *value)
> +{
> +    gchar *p, *p2, quote;
> +
> +    /* Trim the string at first space or semicolon if it is not enclosed
> in
> +     * single or double quotes. */
> +    if ((value[0] != '"') || (value[0] == '\'')) {
> +        p = strchr(value, ' ');
> +        if (p != NULL) {
> +            *p = 0;
> +        }
> +        p = strchr(value, ';');
> +        if (p != NULL) {
> +            *p = 0;
> +        }
> +        return;
> +    }
> +
> +    quote = value[0];
> +    p2 = value;
> +    p = value + 1;
> +    while (*p != 0) {
> +        if (*p == '\\') {
> +            p++;
> +            switch (*p) {
> +            case '$':
> +            case '\'':
> +            case '"':
> +            case '\\':
> +            case '`':
> +                break;
> +            default:
> +                /* Keep literal backslash followed by whatever is there */
> +                p--;
> +                break;
> +            }
> +        } else if (*p == quote) {
> +            *p2 = 0;
> +            break;
> +        }
> +        *(p2++) = *(p++);
> +    }
> +}
> +
> +static GKeyFile *ga_parse_osrelease(const char *fname)
> +{
> +    gboolean ret;
> +    gchar *content = NULL;
> +    gchar *content2 = NULL;
> +    gsize len;
> +    GError *err = NULL;
> +    GKeyFile *keys = g_key_file_new();
> +    const char *group = "[os-release]\n";
> +
> +    ret = g_file_get_contents(fname, &content, &len, &err);
> +    if (ret != TRUE) {
>

I would rather have

if (!g_file_get_contents(..))

same for the calls below,


> +        slog("failed to read '%s', error: %s", fname, err->message);
> +        goto fail;
> +    }
> +
> +    ret = g_utf8_validate(content, len, NULL);
> +    if (ret != TRUE) {
> +        slog("file is not utf-8 encoded: %s", fname);
> +        goto fail;
> +    }
> +    content2 = g_strdup_printf("%s%s", group, content);
> +    len += strlen(group);
>

I wouldn't bother with len, and pass -1 to load_from_data


> +
> +    ret = g_key_file_load_from_data(keys, content2, len, G_KEY_FILE_NONE,
> +        &err);
> +    if (ret != TRUE) {
> +        slog("failed to parse file '%s', error: %s", fname, err->message);
> +        goto fail;
> +    }
> +
> +    g_free(content);
> +    g_free(content2);
> +    return keys;
> +
> +fail:
> +    g_error_free(err);
> +    g_free(content);
> +    g_free(content2);
> +    g_key_file_free(keys);
> +    return NULL;
> +}
> +
> +GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> +{
> +    GuestOSInfo *info = g_new0(GuestOSInfo, 1);
> +
> +    struct utsname kinfo = {0};
> +    if (uname(&kinfo) != 0) {
> +        error_setg_errno(errp, errno, "uname failed");
>

leaks info here

Alternatively, you could make it non-fatal and keep going with a warning
only?


> +        return NULL;
> +    }
> +
> +    info->kernel_version = g_strdup(kinfo.version);
> +    info->kernel_release = g_strdup(kinfo.release);
> +    info->machine_hardware = g_strdup(kinfo.machine);
> +
> +    GKeyFile *osrelease = ga_parse_osrelease("/etc/os-release");
> +    if (osrelease == NULL) {
> +        osrelease = ga_parse_osrelease("/usr/lib/os-release");
> +    }
> +
> +    if (osrelease != NULL) {
> +        char *value;
> +
> +#define GET_FIELD(field, osfield) do { \
> +    value = g_key_file_get_value(osrelease, "os-release", osfield, NULL);
> \
> +    if (value != NULL) { \
> +        ga_osrelease_replace_special(value); \
> +        info->has_ ## field = true; \
> +        info->field = value; \
> +    } \
> +} while (0)
> +        GET_FIELD(id, "ID");
> +        GET_FIELD(name, "NAME");
> +        GET_FIELD(pretty_name, "PRETTY_NAME");
> +        GET_FIELD(version, "VERSION");
> +        GET_FIELD(version_id, "VERSION_ID");
> +        GET_FIELD(variant, "VARIANT");
> +        GET_FIELD(variant_id, "VARIANT_ID");
> +#undef GET_FIELD
> +
> +        g_key_file_free(osrelease);
> +    }
> +
> +    return info;
> +}
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 439d229225..e471f5561b 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1639,3 +1639,188 @@ GuestUserList *qmp_guest_get_users(Error **err)
>      return NULL;
>  #endif
>  }
> +
> +typedef struct _ga_matrix_lookup_t {
> +    int major;
> +    int minor;
> +    char const *version;
> +    char const *version_id;
> +} ga_matrix_lookup_t;
> +
> +static ga_matrix_lookup_t const WIN_VERSION_MATRIX[2][8] = {
> +    {
> +        /* Desktop editions */
> +        { 5, 0, "Microsoft Windows 2000",   "2000"},
> +        { 5, 1, "Microsoft Windows XP",     "xp"},
> +        { 6, 0, "Microsoft Windows Vista",  "vista"},
> +        { 6, 1, "Microsoft Windows 7"       "7"},
> +        { 6, 2, "Microsoft Windows 8",      "8"},
> +        { 6, 3, "Microsoft Windows 8.1",    "8.1"},
> +        {10, 0, "Microsoft Windows 10",     "10"},
> +        { 0, 0, 0}
> +    },{
> +        /* Server editions */
> +        { 5, 2, "Microsoft Windows Server 2003",        "2003"},
> +        { 6, 0, "Microsoft Windows Server 2008",        "2008"},
> +        { 6, 1, "Microsoft Windows Server 2008 R2",     "2008r2"},
> +        { 6, 2, "Microsoft Windows Server 2012",        "2012"},
> +        { 6, 3, "Microsoft Windows Server 2012 R2",     "2012r2"},
> +        {10, 0, "Microsoft Windows Server 2016",        "2016"},
> +        { 0, 0, 0},
> +        { 0, 0, 0}
> +    }
> +};
> +
> +static void ga_get_win_version(RTL_OSVERSIONINFOEXW *info, Error **errp)
> +{
> +    typedef NTSTATUS(WINAPI * rtl_get_version_t)(
> +        RTL_OSVERSIONINFOEXW *os_version_info_ex);
> +
> +    info->dwOSVersionInfoSize = sizeof(RTL_OSVERSIONINFOEXW);
> +
> +    HMODULE module = GetModuleHandle("ntdll");
> +    PVOID fun = GetProcAddress(module, "RtlGetVersion");
> +    if (fun == NULL) {
> +        error_setg(errp, QERR_QGA_COMMAND_FAILED,
> +            "Failed to get address of RtlGetVersion");
> +        return;
> +    }
> +
> +    rtl_get_version_t rtl_get_version = (rtl_get_version_t)fun;
> +    rtl_get_version(info);
> +    return;
> +}
> +
> +static char *ga_get_win_name(OSVERSIONINFOEXW const *os_version, bool id)
> +{
> +    DWORD major = os_version->dwMajorVersion;
> +    DWORD minor = os_version->dwMinorVersion;
> +    int tbl_idx = (os_version->wProductType != VER_NT_WORKSTATION);
> +    ga_matrix_lookup_t const *table = WIN_VERSION_MATRIX[tbl_idx];
> +    while (table->version != NULL) {
> +        if (major == table->major && minor == table->minor) {
> +            if (id) {
> +                return g_strdup(table->version_id);
> +            } else {
> +                return g_strdup(table->version);
> +            }
> +        }
> +        ++table;
> +    }
> +    slog("failed to lookup Windows version: major=%lu, minor=%lu",
> +        major, minor);
> +    return g_strdup("N/A");
> +}
> +
> +static char *ga_get_win_product_name(Error **errp)
> +{
> +    HKEY key = NULL;
> +    DWORD size = 128;
> +    char *result = g_malloc0(size);
> +    LONG err = ERROR_SUCCESS;
> +
> +    err = RegOpenKeyA(HKEY_LOCAL_MACHINE,
> +                      "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion",
> +                      &key);
> +    if (err != ERROR_SUCCESS) {
> +        error_setg_win32(errp, err, "failed to open registry key");
> +        goto fail;
> +    }
> +
> +    err = RegQueryValueExA(key, "ProductName", NULL, NULL,
> +                            (LPBYTE)result, &size);
> +    if (err == ERROR_MORE_DATA) {
> +        slog("ProductName longer than expected (%lu bytes), retrying",
> +                size);
> +        g_free(result);
> +        result = NULL;
> +        if (size > 0) {
> +            result = g_malloc0(size);
> +            err = RegQueryValueExA(key, "ProductName", NULL, NULL,
> +                                    (LPBYTE)result, &size);
> +        }
> +    }
> +    if (err != ERROR_SUCCESS) {
> +        error_setg_win32(errp, err, "failed to retrive ProductName");
> +        goto fail;
> +    }
> +
> +    return result;
> +
> +fail:
> +    g_free(result);
> +    return NULL;
> +}
> +
> +static char *ga_get_current_arch(void)
> +{
> +    SYSTEM_INFO info;
> +    GetNativeSystemInfo(&info);
> +    char *result = NULL;
> +    switch (info.wProcessorArchitecture) {
> +    case PROCESSOR_ARCHITECTURE_AMD64:
> +        result = g_strdup("x86_64");
> +        break;
> +    case PROCESSOR_ARCHITECTURE_ARM:
> +        result = g_strdup("arm");
> +        break;
> +    case PROCESSOR_ARCHITECTURE_IA64:
> +        result = g_strdup("ia64");
> +        break;
> +    case PROCESSOR_ARCHITECTURE_INTEL:
> +        result = g_strdup("x86");
> +        break;
> +    case PROCESSOR_ARCHITECTURE_UNKNOWN:
> +    default:
> +        slog("unknown processor architecture 0x%0x",
> +            info.wProcessorArchitecture);
> +        result = g_strdup("unknown");
> +        break;
> +    }
> +    return result;
> +}
> +
> +GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> +{
> +    Error *local_err = NULL;
> +    OSVERSIONINFOEXW os_version = {0};
> +
> +    ga_get_win_version(&os_version, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return NULL;
> +    }
> +
> +    bool server = os_version.wProductType != VER_NT_WORKSTATION;
> +    char *product_name = ga_get_win_product_name(&local_err);
> +    if (product_name == NULL) {
> +        error_propagate(errp, local_err);
> +        return NULL;
> +    }
> +
> +    GuestOSInfo *info = g_new0(GuestOSInfo, 1);
> +
> +    info->kernel_version = g_strdup_printf("%lu.%lu",
> +        os_version.dwMajorVersion,
> +        os_version.dwMinorVersion);
> +    info->kernel_release = g_strdup_printf("%lu",
> +        os_version.dwBuildNumber);
> +    info->machine_hardware = ga_get_current_arch();
> +
> +    info->has_id = true;
> +    info->id = g_strdup("mswindows");
> +    info->has_name = true;
> +    info->name = g_strdup("Microsoft Windows");
> +    info->has_pretty_name = true;
> +    info->pretty_name = product_name;
> +    info->has_version = true;
> +    info->version = ga_get_win_name(&os_version, false);
> +    info->has_version_id = true;
> +    info->version_id = ga_get_win_name(&os_version, true);
> +    info->has_variant = true;
> +    info->variant = g_strdup(server ? "server" : "client");
> +    info->has_variant_id = true;
> +    info->variant_id = g_strdup(server ? "server" : "client");
> +
> +    return info;
> +}
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 03743ab905..3d9478c10b 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1126,3 +1126,68 @@
>  ##
>  { 'command': 'guest-get-timezone',
>    'returns': 'GuestTimezone' }
> +
> +##
> +# @GuestOSInfo:
> +#
> +# @kernel-release:
> +#     * _POSIX:_ release field returned by uname(2)
> +#     * _Windows:_ version number of the OS
>

Does _foo_ really make the result more clear, I am not convinced, * foo:
already stands out imho.

+# @kernel-version:
> +#     * _POSIX:_ version field returned by uname(2)
> +#     * _Windows:_ build number of the OS
> +# @machine-hardware:
> +#     * _POSIX:_ machine field returned by uname(2)
> +#     * _Windows:_ architecture of the hardware
> +# @id:
> +#     * _POSIX:_ as defined by os-release(5)
> +#     * _Windows:_ contains string "mswindows"
> +# @name:
> +#     * _POSIX:_ as defined by os-release(5)
> +#     * _Windows:_ contains string "Microsoft Windows"
> +# @pretty-name:
> +#     * _POSIX:_ as defined by os-release(5)
> +#     * _Windows:_ product name, e.g. "Microsoft Windows 10 Enterprise"
> +# @version:
> +#     * _POSIX:_ as defined by os-release(5)
> +#     * _Windows:_ long version string, e.g. "Microsoft Windows Server
> 2008"
> +# @version-id:
> +#     * _POSIX:_ as defined by os-release(5)
> +#     * _Windows:_ short version identifier, e.g. "7" or "20012r2"
> +# @variant:
> +#     * _POSIX:_ as defined by os-release(5)
> +#     * _Windows:_ contains string "server" or "client"
> +# @variant-id:
> +#     * _POSIX:_ as defined by os-release(5)
> +#     * _Windows:_ contains string "server" or "client"
> +#
> +# Notes:
> +#
> +# On POSIX systems the fields @id, @name, @pretty-name, @version,
> @version-id,
> +# @variant and @variant-id follow the definition specified in
> os-release(5).
> +# Refer to the manual page for exact description of the fields. Their
> values
> +# are taken from the os-release file. If the file is not present in the
> system,
> +# or the values are not present in the file, the fields are not included.
> +#
> +# On Windows the values are filled from information gathered from the
> system.
> +#
> +# Since: 2.10
> +##
> +{ 'struct': 'GuestOSInfo',
> +  'data': {
> +      'kernel-release': 'str', 'kernel-version': 'str',
> +      'machine-hardware': 'str', '*id': 'str', '*name': 'str',
> +      '*pretty-name': 'str', '*version': 'str', '*version-id': 'str',
> +      '*variant': 'str', '*variant-id': 'str' } }
> +
> +##
> +# @guest-get-osinfo:
> +#
> +# Retrieve guest operating system information
> +#
> +# Returns: @GuestOSInfo
> +#
> +# Since: 2.10
> +##
> +{ 'command': 'guest-get-osinfo',
> +  'returns': 'GuestOSInfo' }
> --
> 2.13.1
>

Looks good to me otherwise
-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v7 3/3] test-qga: add test for guest-get-osinfo

[Marc-André Lureau]

Hi

On Mon, Jul 3, 2017 at 1:40 PM Tomáš Golembiovský <tgolembi@redhat.com>
wrote:

> Add test for guest-get-osinfo command.
>
> Qemu-ga was modified to accept QGA_OS_RELEASE environment variable. If
> the variable is defined it is interpreted as path to the os-release file
> and it is parsed instead of the default paths.
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-posix.c           | 13 +++++++---
>  tests/data/test-qga-os-release |  7 ++++++
>  tests/test-qga.c               | 55
> ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 72 insertions(+), 3 deletions(-)
>  create mode 100644 tests/data/test-qga-os-release
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7a933a5f12..16b18c96ef 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2683,9 +2683,16 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>      info->kernel_release = g_strdup(kinfo.release);
>      info->machine_hardware = g_strdup(kinfo.machine);
>
> -    GKeyFile *osrelease = ga_parse_osrelease("/etc/os-release");
> -    if (osrelease == NULL) {
> -        osrelease = ga_parse_osrelease("/usr/lib/os-release");
> +    GKeyFile *osrelease = NULL;
> +
> +    const char *qga_os_release = g_getenv("QGA_OS_RELEASE");
> +    if (qga_os_release != NULL) {
> +        osrelease = ga_parse_osrelease(qga_os_release);
> +    } else {
> +        osrelease = ga_parse_osrelease("/etc/os-release");
> +        if (osrelease == NULL) {
> +            osrelease = ga_parse_osrelease("/usr/lib/os-release");
> +        }
>      }
>
>      if (osrelease != NULL) {
> diff --git a/tests/data/test-qga-os-release
> b/tests/data/test-qga-os-release
> new file mode 100644
> index 0000000000..70664eb6ec
> --- /dev/null
> +++ b/tests/data/test-qga-os-release
> @@ -0,0 +1,7 @@
> +ID=qemu-ga-test
> +NAME=QEMU-GA
> +PRETTY_NAME="QEMU Guest Agent test"
> +VERSION="Test 1"
> +VERSION_ID=1
> +VARIANT="Unit test \"\'\$\`\\ and \\\\ etc."
> +VARIANT_ID=unit-test
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index 631b98639a..c151dfdc34 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -936,6 +936,59 @@ static void test_qga_guest_exec_invalid(gconstpointer
> fix)
>      QDECREF(ret);
>  }
>
> +static void test_qga_guest_get_osinfo(gconstpointer data)
> +{
> +    TestFixture fixture;
> +    const gchar *str;
> +    gchar *cwd, *env[2];
> +    QDict *ret, *val;
> +
> +    cwd = g_get_current_dir();
> +    env[0] = g_strdup_printf(
> +        "QGA_OS_RELEASE=%s%ctests%cdata%ctest-qga-os-release",
> +        cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
> +    env[1] = NULL;
> +    g_free(cwd);
> +    fixture_setup(&fixture, NULL, env);
> +
> +    ret = qmp_fd(fixture.fd, "{'execute': 'guest-get-osinfo'}");
> +    g_assert_nonnull(ret);
> +    qmp_assert_no_error(ret);
> +
> +    val = qdict_get_qdict(ret, "return");
> +
> +    str = qdict_get_try_str(val, "id");
> +    g_assert_nonnull(str);
> +    g_assert_cmpstr(str, ==, "qemu-ga-test");
> +
> +    str = qdict_get_try_str(val, "name");
> +    g_assert_nonnull(str);
> +    g_assert_cmpstr(str, ==, "QEMU-GA");
> +
> +    str = qdict_get_try_str(val, "pretty-name");
> +    g_assert_nonnull(str);
> +    g_assert_cmpstr(str, ==, "QEMU Guest Agent test");
> +
> +    str = qdict_get_try_str(val, "version");
> +    g_assert_nonnull(str);
> +    g_assert_cmpstr(str, ==, "Test 1");
> +
> +    str = qdict_get_try_str(val, "version-id");
> +    g_assert_nonnull(str);
> +    g_assert_cmpstr(str, ==, "1");
> +
> +    str = qdict_get_try_str(val, "variant");
> +    g_assert_nonnull(str);
> +    g_assert_cmpstr(str, ==, "Unit test \"'$`\\ and \\\\ etc.");
> +
> +    str = qdict_get_try_str(val, "variant-id");
> +    g_assert_nonnull(str);
> +    g_assert_cmpstr(str, ==, "unit-test");
> +
> +    QDECREF(ret);
> +    g_free(env[0]);
>

 +    fixture_tear_down(&fixture, NULL);

+}
> +
>  int main(int argc, char **argv)
>  {
>      TestFixture fix;
> @@ -972,6 +1025,8 @@ int main(int argc, char **argv)
>      g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
>      g_test_add_data_func("/qga/guest-exec-invalid", &fix,
>                           test_qga_guest_exec_invalid);
> +    g_test_add_data_func("/qga/guest-get-osinfo", &fix,
> +                         test_qga_guest_get_osinfo);
>
>      if (g_getenv("QGA_TEST_SIDE_EFFECTING")) {
>          g_test_add_data_func("/qga/fsfreeze-and-thaw", &fix,
> --
> 2.13.1
>
>
With that

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

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v7 1/3] qemu-ga: add guest-get-osinfo command

[Tomáš Golembiovský]

Hi,

thanks for the review. Couple notes below.


On Mon, 03 Jul 2017 14:30:22 +0000
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> On Mon, Jul 3, 2017 at 1:40 PM Tomáš Golembiovský <tgolembi@redhat.com>
> wrote:
> 
> > Add a new 'guest-get-osinfo' command for reporting basic information of
> > the guest operating system. This includes machine architecture,
> > version and release of the kernel and several fields from os-release
> > file if it is present (as defined in [1]).
> >
> > [1] https://www.freedesktop.org/software/systemd/man/os-release.html
> >
> > Signed-off-by: Vinzenz Feenstra <vfeenstr@redhat.com>
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> >  qga/commands-posix.c | 136 +++++++++++++++++++++++++++++++++++++
> >  qga/commands-win32.c | 185
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qga/qapi-schema.json |  65 ++++++++++++++++++
> >  3 files changed, 386 insertions(+)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index d8e412275e..7a933a5f12 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -13,6 +13,7 @@
> >
> >  #include "qemu/osdep.h"
> >  #include <sys/ioctl.h>
> > +#include <sys/utsname.h>
> >  #include <sys/wait.h>
> >  #include <dirent.h>
> >  #include <utmpx.h>
> > @@ -2577,3 +2578,138 @@ GuestUserList *qmp_guest_get_users(Error **err)
> >      g_hash_table_destroy(cache);
> >      return head;
> >  }
> > +
> > +/* Replace escaped special characters with theire real values. The
> > replacement
> > + * is done in place -- returned value is in the original string.
> > + */
> > +static void ga_osrelease_replace_special(gchar *value)
> > +{
> > +    gchar *p, *p2, quote;
> > +
> > +    /* Trim the string at first space or semicolon if it is not enclosed
> > in
> > +     * single or double quotes. */
> > +    if ((value[0] != '"') || (value[0] == '\'')) {
> > +        p = strchr(value, ' ');
> > +        if (p != NULL) {
> > +            *p = 0;
> > +        }
> > +        p = strchr(value, ';');
> > +        if (p != NULL) {
> > +            *p = 0;
> > +        }
> > +        return;
> > +    }
> > +
> > +    quote = value[0];
> > +    p2 = value;
> > +    p = value + 1;
> > +    while (*p != 0) {
> > +        if (*p == '\\') {
> > +            p++;
> > +            switch (*p) {
> > +            case '$':
> > +            case '\'':
> > +            case '"':
> > +            case '\\':
> > +            case '`':
> > +                break;
> > +            default:
> > +                /* Keep literal backslash followed by whatever is there */
> > +                p--;
> > +                break;
> > +            }
> > +        } else if (*p == quote) {
> > +            *p2 = 0;
> > +            break;
> > +        }
> > +        *(p2++) = *(p++);
> > +    }
> > +}
> > +
> > +static GKeyFile *ga_parse_osrelease(const char *fname)
> > +{
> > +    gboolean ret;
> > +    gchar *content = NULL;
> > +    gchar *content2 = NULL;
> > +    gsize len;
> > +    GError *err = NULL;
> > +    GKeyFile *keys = g_key_file_new();
> > +    const char *group = "[os-release]\n";
> > +
> > +    ret = g_file_get_contents(fname, &content, &len, &err);
> > +    if (ret != TRUE) {
> >  
> 
> I would rather have
> 
> if (!g_file_get_contents(..))
> 
> same for the calls below,
> 
> 
> > +        slog("failed to read '%s', error: %s", fname, err->message);
> > +        goto fail;
> > +    }
> > +
> > +    ret = g_utf8_validate(content, len, NULL);
> > +    if (ret != TRUE) {
> > +        slog("file is not utf-8 encoded: %s", fname);
> > +        goto fail;
> > +    }
> > +    content2 = g_strdup_printf("%s%s", group, content);
> > +    len += strlen(group);
> >  
> 
> I wouldn't bother with len, and pass -1 to load_from_data
> 
> 
> > +
> > +    ret = g_key_file_load_from_data(keys, content2, len, G_KEY_FILE_NONE,
> > +        &err);
> > +    if (ret != TRUE) {
> > +        slog("failed to parse file '%s', error: %s", fname, err->message);
> > +        goto fail;
> > +    }
> > +
> > +    g_free(content);
> > +    g_free(content2);
> > +    return keys;
> > +
> > +fail:
> > +    g_error_free(err);
> > +    g_free(content);
> > +    g_free(content2);
> > +    g_key_file_free(keys);
> > +    return NULL;
> > +}
> > +
> > +GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> > +{
> > +    GuestOSInfo *info = g_new0(GuestOSInfo, 1);
> > +
> > +    struct utsname kinfo = {0};
> > +    if (uname(&kinfo) != 0) {
> > +        error_setg_errno(errp, errno, "uname failed");
> >  
> 
> leaks info here
> 
> Alternatively, you could make it non-fatal and keep going with a warning
> only?

The fields kernel_version, kernel_release and machine_hardware are
marked as mandatory in the schema. I assumed it's better to return an
error instead of empty strings in these fields.

Otherwise we could just mark those fields as optional too. But then we
can and up with empty response in the unlikely case where uname fails
and there is no os-release file.


> > +        return NULL;
> > +    }
> > +
> > +    info->kernel_version = g_strdup(kinfo.version);
> > +    info->kernel_release = g_strdup(kinfo.release);
> > +    info->machine_hardware = g_strdup(kinfo.machine);
> > +
> > +    GKeyFile *osrelease = ga_parse_osrelease("/etc/os-release");
> > +    if (osrelease == NULL) {
> > +        osrelease = ga_parse_osrelease("/usr/lib/os-release");
> > +    }
> > +
> > +    if (osrelease != NULL) {
> > +        char *value;
> > +
> > +#define GET_FIELD(field, osfield) do { \
> > +    value = g_key_file_get_value(osrelease, "os-release", osfield, NULL);
> > \
> > +    if (value != NULL) { \
> > +        ga_osrelease_replace_special(value); \
> > +        info->has_ ## field = true; \
> > +        info->field = value; \
> > +    } \
> > +} while (0)
> > +        GET_FIELD(id, "ID");
> > +        GET_FIELD(name, "NAME");
> > +        GET_FIELD(pretty_name, "PRETTY_NAME");
> > +        GET_FIELD(version, "VERSION");
> > +        GET_FIELD(version_id, "VERSION_ID");
> > +        GET_FIELD(variant, "VARIANT");
> > +        GET_FIELD(variant_id, "VARIANT_ID");
> > +#undef GET_FIELD
> > +
> > +        g_key_file_free(osrelease);
> > +    }
> > +
> > +    return info;
> > +}
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 439d229225..e471f5561b 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -1639,3 +1639,188 @@ GuestUserList *qmp_guest_get_users(Error **err)
> >      return NULL;
> >  #endif
> >  }
> > +
> > +typedef struct _ga_matrix_lookup_t {
> > +    int major;
> > +    int minor;
> > +    char const *version;
> > +    char const *version_id;
> > +} ga_matrix_lookup_t;
> > +
> > +static ga_matrix_lookup_t const WIN_VERSION_MATRIX[2][8] = {
> > +    {
> > +        /* Desktop editions */
> > +        { 5, 0, "Microsoft Windows 2000",   "2000"},
> > +        { 5, 1, "Microsoft Windows XP",     "xp"},
> > +        { 6, 0, "Microsoft Windows Vista",  "vista"},
> > +        { 6, 1, "Microsoft Windows 7"       "7"},
> > +        { 6, 2, "Microsoft Windows 8",      "8"},
> > +        { 6, 3, "Microsoft Windows 8.1",    "8.1"},
> > +        {10, 0, "Microsoft Windows 10",     "10"},
> > +        { 0, 0, 0}
> > +    },{
> > +        /* Server editions */
> > +        { 5, 2, "Microsoft Windows Server 2003",        "2003"},
> > +        { 6, 0, "Microsoft Windows Server 2008",        "2008"},
> > +        { 6, 1, "Microsoft Windows Server 2008 R2",     "2008r2"},
> > +        { 6, 2, "Microsoft Windows Server 2012",        "2012"},
> > +        { 6, 3, "Microsoft Windows Server 2012 R2",     "2012r2"},
> > +        {10, 0, "Microsoft Windows Server 2016",        "2016"},
> > +        { 0, 0, 0},
> > +        { 0, 0, 0}
> > +    }
> > +};
> > +
> > +static void ga_get_win_version(RTL_OSVERSIONINFOEXW *info, Error **errp)
> > +{
> > +    typedef NTSTATUS(WINAPI * rtl_get_version_t)(
> > +        RTL_OSVERSIONINFOEXW *os_version_info_ex);
> > +
> > +    info->dwOSVersionInfoSize = sizeof(RTL_OSVERSIONINFOEXW);
> > +
> > +    HMODULE module = GetModuleHandle("ntdll");
> > +    PVOID fun = GetProcAddress(module, "RtlGetVersion");
> > +    if (fun == NULL) {
> > +        error_setg(errp, QERR_QGA_COMMAND_FAILED,
> > +            "Failed to get address of RtlGetVersion");
> > +        return;
> > +    }
> > +
> > +    rtl_get_version_t rtl_get_version = (rtl_get_version_t)fun;
> > +    rtl_get_version(info);
> > +    return;
> > +}
> > +
> > +static char *ga_get_win_name(OSVERSIONINFOEXW const *os_version, bool id)
> > +{
> > +    DWORD major = os_version->dwMajorVersion;
> > +    DWORD minor = os_version->dwMinorVersion;
> > +    int tbl_idx = (os_version->wProductType != VER_NT_WORKSTATION);
> > +    ga_matrix_lookup_t const *table = WIN_VERSION_MATRIX[tbl_idx];
> > +    while (table->version != NULL) {
> > +        if (major == table->major && minor == table->minor) {
> > +            if (id) {
> > +                return g_strdup(table->version_id);
> > +            } else {
> > +                return g_strdup(table->version);
> > +            }
> > +        }
> > +        ++table;
> > +    }
> > +    slog("failed to lookup Windows version: major=%lu, minor=%lu",
> > +        major, minor);
> > +    return g_strdup("N/A");
> > +}
> > +
> > +static char *ga_get_win_product_name(Error **errp)
> > +{
> > +    HKEY key = NULL;
> > +    DWORD size = 128;
> > +    char *result = g_malloc0(size);
> > +    LONG err = ERROR_SUCCESS;
> > +
> > +    err = RegOpenKeyA(HKEY_LOCAL_MACHINE,
> > +                      "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion",
> > +                      &key);
> > +    if (err != ERROR_SUCCESS) {
> > +        error_setg_win32(errp, err, "failed to open registry key");
> > +        goto fail;
> > +    }
> > +
> > +    err = RegQueryValueExA(key, "ProductName", NULL, NULL,
> > +                            (LPBYTE)result, &size);
> > +    if (err == ERROR_MORE_DATA) {
> > +        slog("ProductName longer than expected (%lu bytes), retrying",
> > +                size);
> > +        g_free(result);
> > +        result = NULL;
> > +        if (size > 0) {
> > +            result = g_malloc0(size);
> > +            err = RegQueryValueExA(key, "ProductName", NULL, NULL,
> > +                                    (LPBYTE)result, &size);
> > +        }
> > +    }
> > +    if (err != ERROR_SUCCESS) {
> > +        error_setg_win32(errp, err, "failed to retrive ProductName");
> > +        goto fail;
> > +    }
> > +
> > +    return result;
> > +
> > +fail:
> > +    g_free(result);
> > +    return NULL;
> > +}
> > +
> > +static char *ga_get_current_arch(void)
> > +{
> > +    SYSTEM_INFO info;
> > +    GetNativeSystemInfo(&info);
> > +    char *result = NULL;
> > +    switch (info.wProcessorArchitecture) {
> > +    case PROCESSOR_ARCHITECTURE_AMD64:
> > +        result = g_strdup("x86_64");
> > +        break;
> > +    case PROCESSOR_ARCHITECTURE_ARM:
> > +        result = g_strdup("arm");
> > +        break;
> > +    case PROCESSOR_ARCHITECTURE_IA64:
> > +        result = g_strdup("ia64");
> > +        break;
> > +    case PROCESSOR_ARCHITECTURE_INTEL:
> > +        result = g_strdup("x86");
> > +        break;
> > +    case PROCESSOR_ARCHITECTURE_UNKNOWN:
> > +    default:
> > +        slog("unknown processor architecture 0x%0x",
> > +            info.wProcessorArchitecture);
> > +        result = g_strdup("unknown");
> > +        break;
> > +    }
> > +    return result;
> > +}
> > +
> > +GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    OSVERSIONINFOEXW os_version = {0};
> > +
> > +    ga_get_win_version(&os_version, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return NULL;
> > +    }
> > +
> > +    bool server = os_version.wProductType != VER_NT_WORKSTATION;
> > +    char *product_name = ga_get_win_product_name(&local_err);
> > +    if (product_name == NULL) {
> > +        error_propagate(errp, local_err);
> > +        return NULL;
> > +    }
> > +
> > +    GuestOSInfo *info = g_new0(GuestOSInfo, 1);
> > +
> > +    info->kernel_version = g_strdup_printf("%lu.%lu",
> > +        os_version.dwMajorVersion,
> > +        os_version.dwMinorVersion);
> > +    info->kernel_release = g_strdup_printf("%lu",
> > +        os_version.dwBuildNumber);
> > +    info->machine_hardware = ga_get_current_arch();
> > +
> > +    info->has_id = true;
> > +    info->id = g_strdup("mswindows");
> > +    info->has_name = true;
> > +    info->name = g_strdup("Microsoft Windows");
> > +    info->has_pretty_name = true;
> > +    info->pretty_name = product_name;
> > +    info->has_version = true;
> > +    info->version = ga_get_win_name(&os_version, false);
> > +    info->has_version_id = true;
> > +    info->version_id = ga_get_win_name(&os_version, true);
> > +    info->has_variant = true;
> > +    info->variant = g_strdup(server ? "server" : "client");
> > +    info->has_variant_id = true;
> > +    info->variant_id = g_strdup(server ? "server" : "client");
> > +
> > +    return info;
> > +}
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index 03743ab905..3d9478c10b 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -1126,3 +1126,68 @@
> >  ##
> >  { 'command': 'guest-get-timezone',
> >    'returns': 'GuestTimezone' }
> > +
> > +##
> > +# @GuestOSInfo:
> > +#
> > +# @kernel-release:
> > +#     * _POSIX:_ release field returned by uname(2)
> > +#     * _Windows:_ version number of the OS
> >  
> 
> Does _foo_ really make the result more clear, I am not convinced, * foo:
> already stands out imho.
> 

Yeah, I can remove that.

To me it just looked better in the generated docs (probably at the
expense of the readability of the schema itself).

    Tomas

> +# @kernel-version:
> > +#     * _POSIX:_ version field returned by uname(2)
> > +#     * _Windows:_ build number of the OS
> > +# @machine-hardware:
> > +#     * _POSIX:_ machine field returned by uname(2)
> > +#     * _Windows:_ architecture of the hardware
> > +# @id:
> > +#     * _POSIX:_ as defined by os-release(5)
> > +#     * _Windows:_ contains string "mswindows"
> > +# @name:
> > +#     * _POSIX:_ as defined by os-release(5)
> > +#     * _Windows:_ contains string "Microsoft Windows"
> > +# @pretty-name:
> > +#     * _POSIX:_ as defined by os-release(5)
> > +#     * _Windows:_ product name, e.g. "Microsoft Windows 10 Enterprise"
> > +# @version:
> > +#     * _POSIX:_ as defined by os-release(5)
> > +#     * _Windows:_ long version string, e.g. "Microsoft Windows Server
> > 2008"
> > +# @version-id:
> > +#     * _POSIX:_ as defined by os-release(5)
> > +#     * _Windows:_ short version identifier, e.g. "7" or "20012r2"
> > +# @variant:
> > +#     * _POSIX:_ as defined by os-release(5)
> > +#     * _Windows:_ contains string "server" or "client"
> > +# @variant-id:
> > +#     * _POSIX:_ as defined by os-release(5)
> > +#     * _Windows:_ contains string "server" or "client"
> > +#
> > +# Notes:
> > +#
> > +# On POSIX systems the fields @id, @name, @pretty-name, @version,
> > @version-id,
> > +# @variant and @variant-id follow the definition specified in
> > os-release(5).
> > +# Refer to the manual page for exact description of the fields. Their
> > values
> > +# are taken from the os-release file. If the file is not present in the
> > system,
> > +# or the values are not present in the file, the fields are not included.
> > +#
> > +# On Windows the values are filled from information gathered from the
> > system.
> > +#
> > +# Since: 2.10
> > +##
> > +{ 'struct': 'GuestOSInfo',
> > +  'data': {
> > +      'kernel-release': 'str', 'kernel-version': 'str',
> > +      'machine-hardware': 'str', '*id': 'str', '*name': 'str',
> > +      '*pretty-name': 'str', '*version': 'str', '*version-id': 'str',
> > +      '*variant': 'str', '*variant-id': 'str' } }
> > +
> > +##
> > +# @guest-get-osinfo:
> > +#
> > +# Retrieve guest operating system information
> > +#
> > +# Returns: @GuestOSInfo
> > +#
> > +# Since: 2.10
> > +##
> > +{ 'command': 'guest-get-osinfo',
> > +  'returns': 'GuestOSInfo' }
> > --
> > 2.13.1
> >  
> 
> Looks good to me otherwise
> -- 
> Marc-André Lureau


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