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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3709 bytes --]

On 03/16/2017 05:23 AM, Vinzenz 'evilissimo' Feenstra wrote:
> From: Vinzenz Feenstra <vfeenstr@redhat.com>
> 
> 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 <vfeenstr@redhat.com>
> ---


> +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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]