Re: [Qemu-devel] [PATCH] qemu-ga: add guest-get-osinfo command

2017-03-21 Thread Vinzenz Feenstra

> On Mar 21, 2017, at 2:39 PM, Eric Blake  wrote:
> 
> On 03/16/2017 05:23 AM, Vinzenz 'evilissimo' Feenstra wrote:
>> From: Vinzenz Feenstra 
>> 
>> Add a new 'guest-get-osinfo' command for reporting basic information of
>> the guest operating system (hereafter just 'OS'). This information
>> includes the type of the OS, the version, and the architecture.
>> Additionally reported would be a name, distribution type and kernel
>> version where applicable.
>> 
>> Here an example for a Fedora 25 VM:
>> 
>> $ virsh -c qemu:system qemu-agent-command F25 \
>>'{ "execute": "guest-get-osinfo" }'
>>  {"return":{"arch":"x86_64","codename":"Server Edition","version":"25",
>>   "kernel":"4.8.6-300.fc25.x86_64","type":"linux","distribution":"Fedora"}}
>> 
>> And an example for a Windows 2012 R2 VM:
>> 
>> $ virsh -c qemu:system qemu-agent-command Win2k12R2 \
>>'{ "execute": "guest-get-osinfo" }'
>>  {"return":{"arch":"x86_64","codename":"Win 2012 R2",
>>   "version":"6.3","kernel":"","type":"windows","distribution":""}}
>> 
>> Signed-off-by: Vinzenz Feenstra 
>> ---
> 
> 
>> +static void ga_get_linux_distribution_info(GuestOSInfo *info)
>> +{
>> +FILE *fp = NULL;
>> +fp = fopen("/etc/os-release", "r");
> 
> Rather than read random files, I'm thinking that the uname() syscall is
> a better approach.  It is portable to more guests (since uname is a
> POSIX interface), even if it is somewhat more limited on the information
> it provides.

How about using it as a fallback option only but still use the information 
available
on the other places. I think it’s useful information what can be found there.

> 
>> +++ b/qga/commands-win32.c
>> @@ -1536,3 +1536,108 @@ void ga_command_state_init(GAState *s, 
>> GACommandState *cs)
>> ga_command_state_add(cs, NULL, guest_fsfreeze_cleanup);
>> }
>> }
> 
>> +static char *ga_get_win_ver(void)
>> +{
>> +OSVERSIONINFOEXW os_version;
>> +ga_get_version(&os_version);
>> +char buf[64] = {};
>> +int major = (int)os_version.dwMajorVersion;
>> +int minor = (int)os_version.dwMinorVersion;
>> +sprintf(buf, "%d.%d", major, minor);
>> +return strdup(buf);
> 
> Instead of using strdup() (which uses malloc()), you should be using
> g_strdup_printf() (which uses g_malloc().  In general, we prefer
> g_malloc/g_free over raw malloc.  And once you use g_strdup_printf, you
> no longer have to worry about whether buf is allocated large enough to
> avoid overflow with sprintf.
Will do with the next version. Thanks for that hint with sprintf I didn’t know 
about that.

> 
> 
>> +++ b/qga/qapi-schema.json
>> @@ -1042,3 +1042,43 @@
>>   'data':{ 'path': 'str', '*arg': ['str'], '*env': ['str'],
>>'*input-data': 'str', '*capture-output': 'bool' },
>>   'returns': 'GuestExec' }
>> +
>> +##
>> +# @GuestOSType:
>> +#
>> +# @linux:Indicator for linux distributions
>> +# @windows:  Indicator for windows versions
>> +# @other:Indicator for any other operating system that is not yet
>> +#explicitly supported
>> +#
>> +# Since: 2.8
> 
> You've missed 2.8 by a long shot. In fact, you've missed hard freeze for
> 2.9, so this needs to be 2.10.

I don’t even know where I was looking for finding that number.

> 
>> +##
>> +{ 'enum': 'GuestOSType', 'data': ['linux', 'windows', 'other'] }
>> +
>> +##
>> +# @GuestOSInfo:
>> +#
>> +# @version:  OS version, e.g. 25 for FC25 etc.
>> +# @distribution: Fedora, Ubuntu, Debian, CentOS...
>> +# @codename: Code name of the OS. e.g. Ubuntu has Xenial Xerus etc.
>> +# @arch: Architecture of the OS e.g. x86, x86_64, ppc64, aarch64...
>> +# @type: Specifies the type of the OS.
>> +# @kernel:   Linux kernel version (Might be used by other OS types too).
>> +#May be empty.
> 
> Rather than printing an empty string, would it be better to make the
> fields optional, and omit them when there is nothing useful to supply?

I tried to keep it as stable as possible, but optional would work for me too, 
if it is preferred.

> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org 


Re: [Qemu-devel] [PATCH] qemu-ga: add guest-get-osinfo command

2017-03-21 Thread Eric Blake
On 03/16/2017 05:23 AM, Vinzenz 'evilissimo' Feenstra wrote:
> From: Vinzenz Feenstra 
> 
> Add a new 'guest-get-osinfo' command for reporting basic information of
> the guest operating system (hereafter just 'OS'). This information
> includes the type of the OS, the version, and the architecture.
> Additionally reported would be a name, distribution type and kernel
> version where applicable.
> 
> Here an example for a Fedora 25 VM:
> 
> $ virsh -c qemu:system qemu-agent-command F25 \
> '{ "execute": "guest-get-osinfo" }'
>   {"return":{"arch":"x86_64","codename":"Server Edition","version":"25",
>"kernel":"4.8.6-300.fc25.x86_64","type":"linux","distribution":"Fedora"}}
> 
> And an example for a Windows 2012 R2 VM:
> 
> $ virsh -c qemu:system qemu-agent-command Win2k12R2 \
> '{ "execute": "guest-get-osinfo" }'
>   {"return":{"arch":"x86_64","codename":"Win 2012 R2",
>"version":"6.3","kernel":"","type":"windows","distribution":""}}
> 
> Signed-off-by: Vinzenz Feenstra 
> ---


> +static void ga_get_linux_distribution_info(GuestOSInfo *info)
> +{
> +FILE *fp = NULL;
> +fp = fopen("/etc/os-release", "r");

Rather than read random files, I'm thinking that the uname() syscall is
a better approach.  It is portable to more guests (since uname is a
POSIX interface), even if it is somewhat more limited on the information
it provides.

> +++ b/qga/commands-win32.c
> @@ -1536,3 +1536,108 @@ void ga_command_state_init(GAState *s, GACommandState 
> *cs)
>  ga_command_state_add(cs, NULL, guest_fsfreeze_cleanup);
>  }
>  }

> +static char *ga_get_win_ver(void)
> +{
> +OSVERSIONINFOEXW os_version;
> +ga_get_version(&os_version);
> +char buf[64] = {};
> +int major = (int)os_version.dwMajorVersion;
> +int minor = (int)os_version.dwMinorVersion;
> +sprintf(buf, "%d.%d", major, minor);
> +return strdup(buf);

Instead of using strdup() (which uses malloc()), you should be using
g_strdup_printf() (which uses g_malloc().  In general, we prefer
g_malloc/g_free over raw malloc.  And once you use g_strdup_printf, you
no longer have to worry about whether buf is allocated large enough to
avoid overflow with sprintf.


> +++ b/qga/qapi-schema.json
> @@ -1042,3 +1042,43 @@
>'data':{ 'path': 'str', '*arg': ['str'], '*env': ['str'],
> '*input-data': 'str', '*capture-output': 'bool' },
>'returns': 'GuestExec' }
> +
> +##
> +# @GuestOSType:
> +#
> +# @linux:Indicator for linux distributions
> +# @windows:  Indicator for windows versions
> +# @other:Indicator for any other operating system that is not yet
> +#explicitly supported
> +#
> +# Since: 2.8

You've missed 2.8 by a long shot. In fact, you've missed hard freeze for
2.9, so this needs to be 2.10.

> +##
> +{ 'enum': 'GuestOSType', 'data': ['linux', 'windows', 'other'] }
> +
> +##
> +# @GuestOSInfo:
> +#
> +# @version:  OS version, e.g. 25 for FC25 etc.
> +# @distribution: Fedora, Ubuntu, Debian, CentOS...
> +# @codename: Code name of the OS. e.g. Ubuntu has Xenial Xerus etc.
> +# @arch: Architecture of the OS e.g. x86, x86_64, ppc64, aarch64...
> +# @type: Specifies the type of the OS.
> +# @kernel:   Linux kernel version (Might be used by other OS types too).
> +#May be empty.

Rather than printing an empty string, would it be better to make the
fields optional, and omit them when there is nothing useful to supply?

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature


[Qemu-devel] [PATCH] qemu-ga: add guest-get-osinfo command

2017-03-21 Thread Vinzenz 'evilissimo' Feenstra
From: Vinzenz Feenstra 

Add a new 'guest-get-osinfo' command for reporting basic information of
the guest operating system (hereafter just 'OS'). This information
includes the type of the OS, the version, and the architecture.
Additionally reported would be a name, distribution type and kernel
version where applicable.

Here an example for a Fedora 25 VM:

$ virsh -c qemu:system qemu-agent-command F25 \
'{ "execute": "guest-get-osinfo" }'
  {"return":{"arch":"x86_64","codename":"Server Edition","version":"25",
   "kernel":"4.8.6-300.fc25.x86_64","type":"linux","distribution":"Fedora"}}

And an example for a Windows 2012 R2 VM:

$ virsh -c qemu:system qemu-agent-command Win2k12R2 \
'{ "execute": "guest-get-osinfo" }'
  {"return":{"arch":"x86_64","codename":"Win 2012 R2",
   "version":"6.3","kernel":"","type":"windows","distribution":""}}

Signed-off-by: Vinzenz Feenstra 
---
 qga/commands-posix.c | 206 +++
 qga/commands-win32.c | 105 ++
 qga/qapi-schema.json |  40 ++
 3 files changed, 351 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 73d93eb..70ec3fb 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include 
+#include 
 #include 
 #include 
 #include "qga/guest-agent-core.h"
@@ -2356,6 +2357,205 @@ GuestMemoryBlockInfo 
*qmp_guest_get_memory_block_info(Error **errp)
 return info;
 }
 
+static void ga_strip_end(char *value)
+{
+size_t value_length = strlen(value);
+while (value_length > 0) {
+switch (value[value_length - 1]) {
+default:
+value_length = 0;
+break;
+case ' ': case '\n': case '\t': case '\'': case '"':
+value[value_length - 1] = 0;
+--value_length;
+break;
+}
+}
+}
+
+static void ga_parse_version_id(char const *value, GuestOSInfo *info)
+{
+if (strlen(value) < 128) {
+char codename[128];
+char version[128];
+
+if (*value == '"') {
+++value;
+}
+
+if (sscanf(value, "%[^(] (%[^)])", version, codename) == 2) {
+/* eg. VERSION="16.04.1 LTS (Xenial Xerus)" */
+info->codename = strdup(codename);
+info->version = strdup(version);
+} else if (sscanf(value, "%[^,] %[^\"]\"", version, codename) == 2) {
+/* eg. VERSION="12.04.5 LTS, Precise Pangolin" */
+info->codename = strdup(codename);
+info->version = strdup(version);
+} else {
+/* Just use the rest */
+info->version = strdup(version);
+info->codename = strdup("");
+}
+}
+}
+
+static void ga_parse_debian_version(FILE *fp, GuestOSInfo *info)
+{
+char *line = NULL;
+size_t n = 0;
+
+if (getline(&line, &n, fp) != -1) {
+ga_strip_end(line);
+info->version = strdup(line);
+info->codename = strdup("");
+info->distribution = strdup("Debian GNU/Linux");
+}
+free(line);
+}
+
+static void ga_parse_redhat_release(FILE *fp, GuestOSInfo *info)
+{
+char *line = NULL;
+size_t n = 0;
+
+if (getline(&line, &n, fp) != -1) {
+char *value = strstr(line, " release ");
+if (value != NULL) {
+*value = 0;
+info->distribution = strdup(line);
+value += 9;
+ga_strip_end(value);
+ga_parse_version_id(value, info);
+}
+}
+free(line);
+}
+
+static void ga_parse_os_release(FILE *fp, GuestOSInfo *info)
+{
+char *line = NULL;
+size_t n = 0;
+while (getline(&line, &n, fp) != -1) {
+char *value = strstr(line, "=");
+if (value != NULL) {
+*value = 0;
+++value;
+ga_strip_end(value);
+
+size_t len = strlen(line);
+if (len == 9 && strcmp(line, "VERSION_ID") == 0) {
+info->version = strdup(value);
+} else if (len == 7 && strcmp(line, "VERSION") == 0) {
+ga_parse_version_id(value, info);
+} else if (len == 4 && strcmp(line, "NAME") == 0) {
+info->distribution = strdup(value);
+}
+}
+}
+free(line);
+}
+
+static char *ga_stripped_strdup(char const *value)
+{
+char *result = NULL;
+while (value && *value == '"') {
+++value;
+}
+result = strdup(value);
+ga_strip_end(result);
+return result;
+}
+
+static void ga_parse_lsb_release(FILE *fp, GuestOSInfo *info)
+{
+char *line = NULL;
+size_t n = 0;
+
+while (getline(&line, &n, fp) != -1) {
+char *value = strstr(line, "=");
+if (value != NULL) {
+*value = 0;
+++value;
+ga_strip_end(value);
+
+size_t len = strlen(line);
+if (len == 15 && strcmp(line, "DISTRIB_RELEASE") == 0) {
+info->version = ga_strippe