> On Mar 21, 2017, at 2:39 PM, Eric Blake <ebl...@redhat.com> wrote: > > On 03/16/2017 05:23 AM, Vinzenz 'evilissimo' Feenstra wrote: >> From: Vinzenz Feenstra <vfeen...@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 <vfeen...@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.
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 <http://libvirt.org/>