[Qemu-devel] [PATCH] qga: add command guest-get-devices for reporting VirtIO devices

Tomáš Golembiovský posted 1 patch 4 years, 7 months ago
Test docker-clang@ubuntu passed
Test FreeBSD passed
Test checkpatch passed
Test docker-mingw@fedora passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/10eeadc9a86a7e3b9fa287c0389f0597f5c6a49b.1567094601.git.tgolembi@redhat.com
Maintainers: Michael Roth <mdroth@linux.vnet.ibm.com>
There is a newer version of this series
qga/commands-posix.c |  11 +++
qga/commands-win32.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
qga/qapi-schema.json |  32 +++++++
3 files changed, 237 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH] qga: add command guest-get-devices for reporting VirtIO devices
Posted by Tomáš Golembiovský 4 years, 7 months ago
Add command for reporting devices on Windows guest. The intent is not so
much to report the devices but more importantly the driver (and its
version) that is assigned to the device.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 qga/commands-posix.c |  11 +++
 qga/commands-win32.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
 qga/qapi-schema.json |  32 +++++++
 3 files changed, 237 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index dfc05f5b8a..9adf8bb520 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2709,6 +2709,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
 
     return 0;
 }
+
 #endif /* CONFIG_FSFREEZE */
 
 #if !defined(CONFIG_FSTRIM)
@@ -2757,6 +2758,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
     blacklist = g_list_append(blacklist, g_strdup("guest-fstrim"));
 #endif
 
+    blacklist = g_list_append(blacklist, g_strdup("guest-get-devices"));
+
     return blacklist;
 }
 
@@ -2977,3 +2980,11 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
 
     return info;
 }
+
+GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+
+    return NULL;
+}
+
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 6b67f16faf..0bb93422c7 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -21,10 +21,11 @@
 #ifdef CONFIG_QGA_NTDDSCSI
 #include <winioctl.h>
 #include <ntddscsi.h>
+#endif
 #include <setupapi.h>
 #include <cfgmgr32.h>
 #include <initguid.h>
-#endif
+#include <devpropdef.h>
 #include <lm.h>
 #include <wtsapi32.h>
 #include <wininet.h>
@@ -38,6 +39,36 @@
 #include "qemu/host-utils.h"
 #include "qemu/base64.h"
 
+
+/* The following should be in devpkey.h, but it isn't */
+DEFINE_DEVPROPKEY(DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5, 0xf1, 0x02,
+    0x60, 0x8c, 0x9e, 0xeb, 0xac, 10);  /* DEVPROP_TYPE_STRING */
+DEFINE_DEVPROPKEY(DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c, 0x4efd, 0x80,
+    0x20, 0x67, 0xd1, 0x46, 0xa8, 0x50, 0xe0, 3);
+    /* DEVPROP_TYPE_STRING_LIST */
+DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d, 0x4094, 0xad,
+    0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 2);  /* DEVPROP_TYPE_FILETIME */
+DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverVersion, 0xa8b865dd, 0x2e3d, 0x4094,
+    0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 3);
+    /* DEVPROP_TYPE_STRING */
+/* The following should be in sal.h, but it isn't */
+#ifndef _Out_writes_bytes_opt_
+#define _Out_writes_bytes_opt_(s)
+#endif
+/* The following shoud be in cfgmgr32.h, but it isn't */
+#ifndef CM_Get_DevNode_Property
+CMAPI CONFIGRET WINAPI CM_Get_DevNode_PropertyW(
+    _In_  DEVINST               dnDevInst,
+    _In_  CONST DEVPROPKEY    * PropertyKey,
+    _Out_ DEVPROPTYPE         * PropertyType,
+    _Out_writes_bytes_opt_(*PropertyBufferSize) PBYTE PropertyBuffer,
+    _Inout_ PULONG              PropertyBufferSize,
+    _In_  ULONG                 ulFlags
+    );
+#define CM_Get_DevNode_Property                  CM_Get_DevNode_PropertyW
+#endif
+
+
 #ifndef SHTDN_REASON_FLAG_PLANNED
 #define SHTDN_REASON_FLAG_PLANNED 0x80000000
 #endif
@@ -2234,3 +2265,165 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
 
     return info;
 }
+
+/*
+ * Safely get device property. Returned strings are using wide characters.
+ * Caller is responsible for freeing the buffer.
+ */
+static LPBYTE cm_get_property(DEVINST devInst, const DEVPROPKEY *propName,
+    PDEVPROPTYPE propType)
+{
+    CONFIGRET cr;
+    LPBYTE buffer = NULL;
+    ULONG bufferLen = 0;
+
+    /* First query for needed space */
+    cr = CM_Get_DevNode_Property(devInst, propName, propType,
+        buffer, &bufferLen, 0);
+    if ((cr != CR_SUCCESS) && (cr != CR_BUFFER_SMALL)) {
+
+        g_debug(
+            "failed to get size of device property, device error code=%lx",
+            cr);
+        return NULL;
+    }
+    buffer = (LPBYTE)g_malloc(bufferLen);
+    cr = CM_Get_DevNode_Property(devInst, propName, propType,
+        buffer, &bufferLen, 0);
+    if (cr != CR_SUCCESS) {
+        g_free(buffer);
+        g_debug(
+            "failed to get device property, device error code=%lx", cr);
+        return NULL;
+    }
+    return buffer;
+}
+
+/*
+ * https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices
+ */
+#define DEVICE_PCI_RE "PCI\\\\VEN_(1AF4|1B36)&DEV_([0-9A-B]{4})(&|$)"
+
+GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
+{
+    GuestDeviceInfoList *head = NULL, *cur_item = NULL, *item = NULL;
+    HDEVINFO dev_info = INVALID_HANDLE_VALUE;
+    SP_DEVINFO_DATA dev_info_data;
+    int i;
+    GError *gerr = NULL;
+    GRegex *device_pci_re = NULL;
+
+    device_pci_re = g_regex_new(DEVICE_PCI_RE,
+        G_REGEX_ANCHORED | G_REGEX_OPTIMIZE, 0,
+        &gerr);
+
+    if (gerr) {
+        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
+        g_error_free(gerr);
+        goto out;
+    }
+
+    dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
+    dev_info = SetupDiGetClassDevs(0, 0, 0, DIGCF_PRESENT | DIGCF_ALLCLASSES);
+    if (dev_info == INVALID_HANDLE_VALUE) {
+        error_setg(errp, "failed to get device tree");
+        goto out;
+    }
+
+    g_debug("enumerating devices");
+    for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
+        LPWSTR name = NULL, hwIDs = NULL, lpValue;
+        bool skip = true;
+        DEVPROPTYPE cmType;
+        SYSTEMTIME stUTC;
+        LPFILETIME pfdDriverDate;
+        LPWSTR driverVersion;
+
+        GuestDeviceInfo *device = g_new0(GuestDeviceInfo, 1);
+
+        name = (LPWSTR)cm_get_property(dev_info_data.DevInst, &DEVPKEY_NAME,
+            &cmType);
+        if (name == NULL) {
+            g_debug("failed to get device description");
+            goto next;
+        }
+        device->driver_name = guest_wctomb_dup(name);
+        g_free(name);
+        g_debug("querying device: %s", device->driver_name);
+
+        hwIDs = (LPWSTR)cm_get_property(dev_info_data.DevInst,
+            &DEVPKEY_Device_HardwareIds, &cmType);
+        if (hwIDs == NULL) {
+            g_debug("failed to get hardware IDs");
+            goto next;
+        }
+        for (lpValue = hwIDs;
+            '\0' != *lpValue;
+            lpValue += lstrlenW(lpValue) + 1) {
+            GMatchInfo *match_info;
+            char *hwID = guest_wctomb_dup(lpValue);
+            /* g_debug("hwID: %s", hwID); */
+            if (!g_regex_match(device_pci_re, hwID, 0, &match_info)) {
+                continue;
+            }
+            skip = false;
+            device->vendor_id = g_match_info_fetch(match_info, 1);
+            device->device_id = g_match_info_fetch(match_info, 2);
+            g_match_info_free(match_info);
+        }
+        free(hwIDs);
+
+        if (skip) {
+            goto next;
+        }
+
+        driverVersion = (LPWSTR)cm_get_property(dev_info_data.DevInst,
+            &DEVPKEY_Device_DriverVersion, &cmType);
+        if (driverVersion == NULL) {
+            g_debug("failed to get driver version");
+            goto next;
+        }
+        device->driver_version = guest_wctomb_dup(driverVersion);
+        free(driverVersion);
+
+        pfdDriverDate = (LPFILETIME)cm_get_property(dev_info_data.DevInst,
+            &DEVPKEY_Device_DriverDate, &cmType);
+        if (driverVersion == NULL) {
+            g_debug("failed to get driver date");
+            goto next;
+        }
+        FileTimeToSystemTime(pfdDriverDate, &stUTC);
+        free(pfdDriverDate);
+        device->driver_date = g_strdup_printf("%02d/%02d/%04d",
+            stUTC.wMonth, stUTC.wDay, stUTC.wYear);
+        g_debug("Driver Version: %s,%s\n", device->driver_date,
+            device->driver_version);
+
+        item = g_new0(GuestDeviceInfoList, 1);
+        item->value = device;
+        if (!cur_item) {
+            head = cur_item = item;
+        } else {
+            cur_item->next = item;
+            cur_item = item;
+        }
+        continue;
+
+next:
+        g_free(device->vendor_id);
+        g_free(device->device_id);
+        g_free(device->driver_date);
+        g_free(device->driver_name);
+        g_free(device->driver_version);
+        g_free(device);
+    }
+
+out:
+
+    if (dev_info != INVALID_HANDLE_VALUE) {
+        SetupDiDestroyDeviceInfoList(dev_info);
+    }
+    g_regex_unref(device_pci_re);
+
+    return head;
+}
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index fb4605cc19..ed73b0b1c6 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1242,3 +1242,35 @@
 ##
 { 'command': 'guest-get-osinfo',
   'returns': 'GuestOSInfo' }
+
+##
+# @GuestDeviceInfo:
+#
+# @vendor-id: vendor ID as hexadecimal string in uper case without 0x prefix
+# @device-id: device ID as hexadecimal string in uper case without 0x prefix
+# @driver-name: name of the associated driver
+# @driver-date: driver release date in format MM/DD/YY
+# @driver-version: driver version
+#
+# Since: 4.1.1
+##
+{ 'struct': 'GuestDeviceInfo',
+  'data': {
+      'vendor-id': 'str',
+      'device-id': 'str',
+      'driver-name': 'str',
+      'driver-date': 'str',
+      'driver-version': 'str'
+      } }
+
+##
+# @guest-get-devices:
+#
+# Retrieve information about device drivers in Windows guest
+#
+# Returns: @GuestOSInfo
+#
+# Since: 4.1.1
+##
+{ 'command': 'guest-get-devices',
+  'returns': ['GuestDeviceInfo'] }
-- 
2.22.0


Re: [Qemu-devel] [PATCH] qga: add command guest-get-devices for reporting VirtIO devices
Posted by Tomáš Golembiovský 4 years, 7 months ago
gentle reminder

On Thu, Aug 29, 2019 at 06:03:21PM +0200, Tomáš Golembiovský wrote:
> Add command for reporting devices on Windows guest. The intent is not so
> much to report the devices but more importantly the driver (and its
> version) that is assigned to the device.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-posix.c |  11 +++
>  qga/commands-win32.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
>  qga/qapi-schema.json |  32 +++++++
>  3 files changed, 237 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index dfc05f5b8a..9adf8bb520 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2709,6 +2709,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>  
>      return 0;
>  }
> +
>  #endif /* CONFIG_FSFREEZE */
>  
>  #if !defined(CONFIG_FSTRIM)
> @@ -2757,6 +2758,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
>      blacklist = g_list_append(blacklist, g_strdup("guest-fstrim"));
>  #endif
>  
> +    blacklist = g_list_append(blacklist, g_strdup("guest-get-devices"));
> +
>      return blacklist;
>  }
>  
> @@ -2977,3 +2980,11 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>  
>      return info;
>  }
> +
> +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +
> +    return NULL;
> +}
> +
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6b67f16faf..0bb93422c7 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -21,10 +21,11 @@
>  #ifdef CONFIG_QGA_NTDDSCSI
>  #include <winioctl.h>
>  #include <ntddscsi.h>
> +#endif
>  #include <setupapi.h>
>  #include <cfgmgr32.h>
>  #include <initguid.h>
> -#endif
> +#include <devpropdef.h>
>  #include <lm.h>
>  #include <wtsapi32.h>
>  #include <wininet.h>
> @@ -38,6 +39,36 @@
>  #include "qemu/host-utils.h"
>  #include "qemu/base64.h"
>  
> +
> +/* The following should be in devpkey.h, but it isn't */
> +DEFINE_DEVPROPKEY(DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5, 0xf1, 0x02,
> +    0x60, 0x8c, 0x9e, 0xeb, 0xac, 10);  /* DEVPROP_TYPE_STRING */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c, 0x4efd, 0x80,
> +    0x20, 0x67, 0xd1, 0x46, 0xa8, 0x50, 0xe0, 3);
> +    /* DEVPROP_TYPE_STRING_LIST */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d, 0x4094, 0xad,
> +    0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 2);  /* DEVPROP_TYPE_FILETIME */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverVersion, 0xa8b865dd, 0x2e3d, 0x4094,
> +    0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 3);
> +    /* DEVPROP_TYPE_STRING */
> +/* The following should be in sal.h, but it isn't */
> +#ifndef _Out_writes_bytes_opt_
> +#define _Out_writes_bytes_opt_(s)
> +#endif
> +/* The following shoud be in cfgmgr32.h, but it isn't */
> +#ifndef CM_Get_DevNode_Property
> +CMAPI CONFIGRET WINAPI CM_Get_DevNode_PropertyW(
> +    _In_  DEVINST               dnDevInst,
> +    _In_  CONST DEVPROPKEY    * PropertyKey,
> +    _Out_ DEVPROPTYPE         * PropertyType,
> +    _Out_writes_bytes_opt_(*PropertyBufferSize) PBYTE PropertyBuffer,
> +    _Inout_ PULONG              PropertyBufferSize,
> +    _In_  ULONG                 ulFlags
> +    );
> +#define CM_Get_DevNode_Property                  CM_Get_DevNode_PropertyW
> +#endif
> +
> +
>  #ifndef SHTDN_REASON_FLAG_PLANNED
>  #define SHTDN_REASON_FLAG_PLANNED 0x80000000
>  #endif
> @@ -2234,3 +2265,165 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>  
>      return info;
>  }
> +
> +/*
> + * Safely get device property. Returned strings are using wide characters.
> + * Caller is responsible for freeing the buffer.
> + */
> +static LPBYTE cm_get_property(DEVINST devInst, const DEVPROPKEY *propName,
> +    PDEVPROPTYPE propType)
> +{
> +    CONFIGRET cr;
> +    LPBYTE buffer = NULL;
> +    ULONG bufferLen = 0;
> +
> +    /* First query for needed space */
> +    cr = CM_Get_DevNode_Property(devInst, propName, propType,
> +        buffer, &bufferLen, 0);
> +    if ((cr != CR_SUCCESS) && (cr != CR_BUFFER_SMALL)) {
> +
> +        g_debug(
> +            "failed to get size of device property, device error code=%lx",
> +            cr);
> +        return NULL;
> +    }
> +    buffer = (LPBYTE)g_malloc(bufferLen);
> +    cr = CM_Get_DevNode_Property(devInst, propName, propType,
> +        buffer, &bufferLen, 0);
> +    if (cr != CR_SUCCESS) {
> +        g_free(buffer);
> +        g_debug(
> +            "failed to get device property, device error code=%lx", cr);
> +        return NULL;
> +    }
> +    return buffer;
> +}
> +
> +/*
> + * https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices
> + */
> +#define DEVICE_PCI_RE "PCI\\\\VEN_(1AF4|1B36)&DEV_([0-9A-B]{4})(&|$)"
> +
> +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> +{
> +    GuestDeviceInfoList *head = NULL, *cur_item = NULL, *item = NULL;
> +    HDEVINFO dev_info = INVALID_HANDLE_VALUE;
> +    SP_DEVINFO_DATA dev_info_data;
> +    int i;
> +    GError *gerr = NULL;
> +    GRegex *device_pci_re = NULL;
> +
> +    device_pci_re = g_regex_new(DEVICE_PCI_RE,
> +        G_REGEX_ANCHORED | G_REGEX_OPTIMIZE, 0,
> +        &gerr);
> +
> +    if (gerr) {
> +        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> +        g_error_free(gerr);
> +        goto out;
> +    }
> +
> +    dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> +    dev_info = SetupDiGetClassDevs(0, 0, 0, DIGCF_PRESENT | DIGCF_ALLCLASSES);
> +    if (dev_info == INVALID_HANDLE_VALUE) {
> +        error_setg(errp, "failed to get device tree");
> +        goto out;
> +    }
> +
> +    g_debug("enumerating devices");
> +    for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
> +        LPWSTR name = NULL, hwIDs = NULL, lpValue;
> +        bool skip = true;
> +        DEVPROPTYPE cmType;
> +        SYSTEMTIME stUTC;
> +        LPFILETIME pfdDriverDate;
> +        LPWSTR driverVersion;
> +
> +        GuestDeviceInfo *device = g_new0(GuestDeviceInfo, 1);
> +
> +        name = (LPWSTR)cm_get_property(dev_info_data.DevInst, &DEVPKEY_NAME,
> +            &cmType);
> +        if (name == NULL) {
> +            g_debug("failed to get device description");
> +            goto next;
> +        }
> +        device->driver_name = guest_wctomb_dup(name);
> +        g_free(name);
> +        g_debug("querying device: %s", device->driver_name);
> +
> +        hwIDs = (LPWSTR)cm_get_property(dev_info_data.DevInst,
> +            &DEVPKEY_Device_HardwareIds, &cmType);
> +        if (hwIDs == NULL) {
> +            g_debug("failed to get hardware IDs");
> +            goto next;
> +        }
> +        for (lpValue = hwIDs;
> +            '\0' != *lpValue;
> +            lpValue += lstrlenW(lpValue) + 1) {
> +            GMatchInfo *match_info;
> +            char *hwID = guest_wctomb_dup(lpValue);
> +            /* g_debug("hwID: %s", hwID); */
> +            if (!g_regex_match(device_pci_re, hwID, 0, &match_info)) {
> +                continue;
> +            }
> +            skip = false;
> +            device->vendor_id = g_match_info_fetch(match_info, 1);
> +            device->device_id = g_match_info_fetch(match_info, 2);
> +            g_match_info_free(match_info);
> +        }
> +        free(hwIDs);
> +
> +        if (skip) {
> +            goto next;
> +        }
> +
> +        driverVersion = (LPWSTR)cm_get_property(dev_info_data.DevInst,
> +            &DEVPKEY_Device_DriverVersion, &cmType);
> +        if (driverVersion == NULL) {
> +            g_debug("failed to get driver version");
> +            goto next;
> +        }
> +        device->driver_version = guest_wctomb_dup(driverVersion);
> +        free(driverVersion);
> +
> +        pfdDriverDate = (LPFILETIME)cm_get_property(dev_info_data.DevInst,
> +            &DEVPKEY_Device_DriverDate, &cmType);
> +        if (driverVersion == NULL) {
> +            g_debug("failed to get driver date");
> +            goto next;
> +        }
> +        FileTimeToSystemTime(pfdDriverDate, &stUTC);
> +        free(pfdDriverDate);
> +        device->driver_date = g_strdup_printf("%02d/%02d/%04d",
> +            stUTC.wMonth, stUTC.wDay, stUTC.wYear);
> +        g_debug("Driver Version: %s,%s\n", device->driver_date,
> +            device->driver_version);
> +
> +        item = g_new0(GuestDeviceInfoList, 1);
> +        item->value = device;
> +        if (!cur_item) {
> +            head = cur_item = item;
> +        } else {
> +            cur_item->next = item;
> +            cur_item = item;
> +        }
> +        continue;
> +
> +next:
> +        g_free(device->vendor_id);
> +        g_free(device->device_id);
> +        g_free(device->driver_date);
> +        g_free(device->driver_name);
> +        g_free(device->driver_version);
> +        g_free(device);
> +    }
> +
> +out:
> +
> +    if (dev_info != INVALID_HANDLE_VALUE) {
> +        SetupDiDestroyDeviceInfoList(dev_info);
> +    }
> +    g_regex_unref(device_pci_re);
> +
> +    return head;
> +}
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index fb4605cc19..ed73b0b1c6 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1242,3 +1242,35 @@
>  ##
>  { 'command': 'guest-get-osinfo',
>    'returns': 'GuestOSInfo' }
> +
> +##
> +# @GuestDeviceInfo:
> +#
> +# @vendor-id: vendor ID as hexadecimal string in uper case without 0x prefix
> +# @device-id: device ID as hexadecimal string in uper case without 0x prefix
> +# @driver-name: name of the associated driver
> +# @driver-date: driver release date in format MM/DD/YY
> +# @driver-version: driver version
> +#
> +# Since: 4.1.1
> +##
> +{ 'struct': 'GuestDeviceInfo',
> +  'data': {
> +      'vendor-id': 'str',
> +      'device-id': 'str',
> +      'driver-name': 'str',
> +      'driver-date': 'str',
> +      'driver-version': 'str'
> +      } }
> +
> +##
> +# @guest-get-devices:
> +#
> +# Retrieve information about device drivers in Windows guest
> +#
> +# Returns: @GuestOSInfo
> +#
> +# Since: 4.1.1
> +##
> +{ 'command': 'guest-get-devices',
> +  'returns': ['GuestDeviceInfo'] }
> -- 
> 2.22.0
> 

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

Re: [Qemu-devel] [PATCH] qga: add command guest-get-devices for reporting VirtIO devices
Posted by Marc-André Lureau 4 years, 7 months ago
Hi

On Thu, Aug 29, 2019 at 8:06 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> Add command for reporting devices on Windows guest. The intent is not so
> much to report the devices but more importantly the driver (and its
> version) that is assigned to the device.
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-posix.c |  11 +++
>  qga/commands-win32.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
>  qga/qapi-schema.json |  32 +++++++
>  3 files changed, 237 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index dfc05f5b8a..9adf8bb520 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2709,6 +2709,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>
>      return 0;
>  }
> +
>  #endif /* CONFIG_FSFREEZE */

extra line


>
>  #if !defined(CONFIG_FSTRIM)
> @@ -2757,6 +2758,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
>      blacklist = g_list_append(blacklist, g_strdup("guest-fstrim"));
>  #endif
>
> +    blacklist = g_list_append(blacklist, g_strdup("guest-get-devices"));
> +
>      return blacklist;
>  }
>
> @@ -2977,3 +2980,11 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
>      return info;
>  }
> +
> +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +
> +    return NULL;
> +}
> +

extra EOF line

> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6b67f16faf..0bb93422c7 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -21,10 +21,11 @@
>  #ifdef CONFIG_QGA_NTDDSCSI
>  #include <winioctl.h>
>  #include <ntddscsi.h>
> +#endif
>  #include <setupapi.h>
>  #include <cfgmgr32.h>
>  #include <initguid.h>
> -#endif
> +#include <devpropdef.h>
>  #include <lm.h>
>  #include <wtsapi32.h>
>  #include <wininet.h>
> @@ -38,6 +39,36 @@
>  #include "qemu/host-utils.h"
>  #include "qemu/base64.h"
>
> +
> +/* The following should be in devpkey.h, but it isn't */
> +DEFINE_DEVPROPKEY(DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5, 0xf1, 0x02,
> +    0x60, 0x8c, 0x9e, 0xeb, 0xac, 10);  /* DEVPROP_TYPE_STRING */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c, 0x4efd, 0x80,
> +    0x20, 0x67, 0xd1, 0x46, 0xa8, 0x50, 0xe0, 3);
> +    /* DEVPROP_TYPE_STRING_LIST */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d, 0x4094, 0xad,
> +    0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 2);  /* DEVPROP_TYPE_FILETIME */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverVersion, 0xa8b865dd, 0x2e3d, 0x4094,
> +    0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 3);

perhaps use a qemu prefix to avoid future clash?

> +    /* DEVPROP_TYPE_STRING */
> +/* The following should be in sal.h, but it isn't */
> +#ifndef _Out_writes_bytes_opt_
> +#define _Out_writes_bytes_opt_(s)
> +#endif

It got added in da215fcf5f001d1fdedf82e2f1407e8ff0b6d453
('include/sal: Update sal definitions').

#ifndef protects it, ok

> +/* The following shoud be in cfgmgr32.h, but it isn't */
> +#ifndef CM_Get_DevNode_Property
> +CMAPI CONFIGRET WINAPI CM_Get_DevNode_PropertyW(
> +    _In_  DEVINST               dnDevInst,
> +    _In_  CONST DEVPROPKEY    * PropertyKey,
> +    _Out_ DEVPROPTYPE         * PropertyType,
> +    _Out_writes_bytes_opt_(*PropertyBufferSize) PBYTE PropertyBuffer,
> +    _Inout_ PULONG              PropertyBufferSize,
> +    _In_  ULONG                 ulFlags
> +    );
> +#define CM_Get_DevNode_Property                  CM_Get_DevNode_PropertyW
> +#endif
> +

#ifndef should protect it from future clash, ok

Did you open bugs for mingw64 updates?

> +
>  #ifndef SHTDN_REASON_FLAG_PLANNED
>  #define SHTDN_REASON_FLAG_PLANNED 0x80000000
>  #endif
> @@ -2234,3 +2265,165 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
>      return info;
>  }
> +
> +/*
> + * Safely get device property. Returned strings are using wide characters.
> + * Caller is responsible for freeing the buffer.
> + */
> +static LPBYTE cm_get_property(DEVINST devInst, const DEVPROPKEY *propName,
> +    PDEVPROPTYPE propType)
> +{
> +    CONFIGRET cr;
> +    LPBYTE buffer = NULL;
> +    ULONG bufferLen = 0;
> +
> +    /* First query for needed space */
> +    cr = CM_Get_DevNode_Property(devInst, propName, propType,
> +        buffer, &bufferLen, 0);

I think qemu-ga win32 code generally prefers to be explicit about A()
vs W(), call the W function explicitely, remove the generic define.

> +    if ((cr != CR_SUCCESS) && (cr != CR_BUFFER_SMALL)) {


This file is not a good example, but in general we avoid the extra
parenthesis. Please drop them.

> +
> +        g_debug(
> +            "failed to get size of device property, device error code=%lx",
> +            cr);

That file uses a mix of slog and g_debug..

I think slog() is higher level and preferable here. At least, try to
make it fit on one line would be nice.

> +        return NULL;
> +    }
> +    buffer = (LPBYTE)g_malloc(bufferLen);

I'd suggest g_malloc0(len+1) to avoid surprises.

Drop the cast.

> +    cr = CM_Get_DevNode_Property(devInst, propName, propType,
> +        buffer, &bufferLen, 0);
> +    if (cr != CR_SUCCESS) {
> +        g_free(buffer);

you could use g_auto declaration these days (I know, it wasn't there
when you started, but now it is ;).

> +        g_debug(
> +            "failed to get device property, device error code=%lx", cr);

same about slog

> +        return NULL;
> +    }
> +    return buffer;
> +}
> +
> +/*
> + * https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices
> + */
> +#define DEVICE_PCI_RE "PCI\\\\VEN_(1AF4|1B36)&DEV_([0-9A-B]{4})(&|$)"
> +
> +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> +{
> +    GuestDeviceInfoList *head = NULL, *cur_item = NULL, *item = NULL;
> +    HDEVINFO dev_info = INVALID_HANDLE_VALUE;
> +    SP_DEVINFO_DATA dev_info_data;
> +    int i;
> +    GError *gerr = NULL;
> +    GRegex *device_pci_re = NULL;
> +
> +    device_pci_re = g_regex_new(DEVICE_PCI_RE,
> +        G_REGEX_ANCHORED | G_REGEX_OPTIMIZE, 0,
> +        &gerr);
> +
> +    if (gerr) {
> +        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> +        g_error_free(gerr);
> +        goto out;
> +    }

given that the rule is static, I think you could simplify error
handling and use g_assert(device_pci_re != NULL) instead.

> +
> +    dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> +    dev_info = SetupDiGetClassDevs(0, 0, 0, DIGCF_PRESENT | DIGCF_ALLCLASSES);
> +    if (dev_info == INVALID_HANDLE_VALUE) {
> +        error_setg(errp, "failed to get device tree");
> +        goto out;
> +    }
> +
> +    g_debug("enumerating devices");
> +    for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
> +        LPWSTR name = NULL, hwIDs = NULL, lpValue;
> +        bool skip = true;
> +        DEVPROPTYPE cmType;
> +        SYSTEMTIME stUTC;
> +        LPFILETIME pfdDriverDate;
> +        LPWSTR driverVersion;
> +
> +        GuestDeviceInfo *device = g_new0(GuestDeviceInfo, 1);
> +
> +        name = (LPWSTR)cm_get_property(dev_info_data.DevInst, &DEVPKEY_NAME,
> +            &cmType);
> +        if (name == NULL) {
> +            g_debug("failed to get device description");
> +            goto next;
> +        }
> +        device->driver_name = guest_wctomb_dup(name);

QMP strings should be utf8 encoded.

No idea why guest_wctomb_dup() was introduced in the first place to use ANSI CP.

Imho, every strings returned by W() functions should go through
g_utf16_to_utf8().


> +        g_free(name);
> +        g_debug("querying device: %s", device->driver_name);
> +
> +        hwIDs = (LPWSTR)cm_get_property(dev_info_data.DevInst,
> +            &DEVPKEY_Device_HardwareIds, &cmType);
> +        if (hwIDs == NULL) {
> +            g_debug("failed to get hardware IDs");
> +            goto next;
> +        }
> +        for (lpValue = hwIDs;
> +            '\0' != *lpValue;
> +            lpValue += lstrlenW(lpValue) + 1) {

ok (alternatively, introduce a function that returns a GStrv to make
caller life easier.

> +            GMatchInfo *match_info;
> +            char *hwID = guest_wctomb_dup(lpValue);
> +            /* g_debug("hwID: %s", hwID); */
> +            if (!g_regex_match(device_pci_re, hwID, 0, &match_info)) {
> +                continue;

leaks hwID

> +            }
> +            skip = false;
> +            device->vendor_id = g_match_info_fetch(match_info, 1);
> +            device->device_id = g_match_info_fetch(match_info, 2);
> +            g_match_info_free(match_info);
> +        }
> +        free(hwIDs);

Let's stick to g_free() as you g_malloc()

> +
> +        if (skip) {
> +            goto next;
> +        }
> +
> +        driverVersion = (LPWSTR)cm_get_property(dev_info_data.DevInst,
> +            &DEVPKEY_Device_DriverVersion, &cmType);
> +        if (driverVersion == NULL) {
> +            g_debug("failed to get driver version");
> +            goto next;
> +        }
> +        device->driver_version = guest_wctomb_dup(driverVersion);

I'd use g_utf16_to_utf8()

> +        free(driverVersion);

g_free()

> +
> +        pfdDriverDate = (LPFILETIME)cm_get_property(dev_info_data.DevInst,
> +            &DEVPKEY_Device_DriverDate, &cmType);
> +        if (driverVersion == NULL) {
> +            g_debug("failed to get driver date");
> +            goto next;
> +        }
> +        FileTimeToSystemTime(pfdDriverDate, &stUTC);
> +        free(pfdDriverDate);

g_free()

> +        device->driver_date = g_strdup_printf("%02d/%02d/%04d",
> +            stUTC.wMonth, stUTC.wDay, stUTC.wYear);
> +        g_debug("Driver Version: %s,%s\n", device->driver_date,
> +            device->driver_version);
> +
> +        item = g_new0(GuestDeviceInfoList, 1);
> +        item->value = device;
> +        if (!cur_item) {
> +            head = cur_item = item;
> +        } else {
> +            cur_item->next = item;
> +            cur_item = item;
> +        }
> +        continue;
> +
> +next:
> +        g_free(device->vendor_id);
> +        g_free(device->device_id);
> +        g_free(device->driver_date);
> +        g_free(device->driver_name);
> +        g_free(device->driver_version);
> +        g_free(device);

Please switch to g_auto

> +    }
> +
> +out:
> +
> +    if (dev_info != INVALID_HANDLE_VALUE) {
> +        SetupDiDestroyDeviceInfoList(dev_info);
> +    }
> +    g_regex_unref(device_pci_re);
> +
> +    return head;
> +}
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index fb4605cc19..ed73b0b1c6 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1242,3 +1242,35 @@
>  ##
>  { 'command': 'guest-get-osinfo',
>    'returns': 'GuestOSInfo' }
> +
> +##
> +# @GuestDeviceInfo:
> +#
> +# @vendor-id: vendor ID as hexadecimal string in uper case without 0x prefix
> +# @device-id: device ID as hexadecimal string in uper case without 0x prefix
> +# @driver-name: name of the associated driver
> +# @driver-date: driver release date in format MM/DD/YY
> +# @driver-version: driver version
> +#
> +# Since: 4.1.1
> +##
> +{ 'struct': 'GuestDeviceInfo',
> +  'data': {
> +      'vendor-id': 'str',
> +      'device-id': 'str',
> +      'driver-name': 'str',
> +      'driver-date': 'str',
> +      'driver-version': 'str'
> +      } }
> +
> +##
> +# @guest-get-devices:
> +#
> +# Retrieve information about device drivers in Windows guest
> +#
> +# Returns: @GuestOSInfo

@GuestDeviceInfo

> +#
> +# Since: 4.1.1
> +##
> +{ 'command': 'guest-get-devices',
> +  'returns': ['GuestDeviceInfo'] }
> --
> 2.22.0
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] qga: add command guest-get-devices for reporting VirtIO devices
Posted by Tomáš Golembiovský 4 years, 7 months ago
On Mon, Sep 16, 2019 at 04:31:49PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Aug 29, 2019 at 8:06 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> >
> > Add command for reporting devices on Windows guest. The intent is not so
> > much to report the devices but more importantly the driver (and its
> > version) that is assigned to the device.
> >
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> >  qga/commands-posix.c |  11 +++
> >  qga/commands-win32.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
> >  qga/qapi-schema.json |  32 +++++++
> >  3 files changed, 237 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index dfc05f5b8a..9adf8bb520 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -2709,6 +2709,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
> >
> >      return 0;
> >  }
> > +
> >  #endif /* CONFIG_FSFREEZE */
> 
> extra line
> 
> 
> >
> >  #if !defined(CONFIG_FSTRIM)
> > @@ -2757,6 +2758,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
> >      blacklist = g_list_append(blacklist, g_strdup("guest-fstrim"));
> >  #endif
> >
> > +    blacklist = g_list_append(blacklist, g_strdup("guest-get-devices"));
> > +
> >      return blacklist;
> >  }
> >
> > @@ -2977,3 +2980,11 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> >
> >      return info;
> >  }
> > +
> > +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> > +{
> > +    error_setg(errp, QERR_UNSUPPORTED);
> > +
> > +    return NULL;
> > +}
> > +
> 
> extra EOF line
> 
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 6b67f16faf..0bb93422c7 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -21,10 +21,11 @@
> >  #ifdef CONFIG_QGA_NTDDSCSI
> >  #include <winioctl.h>
> >  #include <ntddscsi.h>
> > +#endif
> >  #include <setupapi.h>
> >  #include <cfgmgr32.h>
> >  #include <initguid.h>
> > -#endif
> > +#include <devpropdef.h>
> >  #include <lm.h>
> >  #include <wtsapi32.h>
> >  #include <wininet.h>
> > @@ -38,6 +39,36 @@
> >  #include "qemu/host-utils.h"
> >  #include "qemu/base64.h"
> >
> > +
> > +/* The following should be in devpkey.h, but it isn't */
> > +DEFINE_DEVPROPKEY(DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5, 0xf1, 0x02,
> > +    0x60, 0x8c, 0x9e, 0xeb, 0xac, 10);  /* DEVPROP_TYPE_STRING */
> > +DEFINE_DEVPROPKEY(DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c, 0x4efd, 0x80,
> > +    0x20, 0x67, 0xd1, 0x46, 0xa8, 0x50, 0xe0, 3);
> > +    /* DEVPROP_TYPE_STRING_LIST */
> > +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d, 0x4094, 0xad,
> > +    0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 2);  /* DEVPROP_TYPE_FILETIME */
> > +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverVersion, 0xa8b865dd, 0x2e3d, 0x4094,
> > +    0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 3);
> 
> perhaps use a qemu prefix to avoid future clash?

Yes, this looks like the only option.

> 
> > +    /* DEVPROP_TYPE_STRING */
> > +/* The following should be in sal.h, but it isn't */
> > +#ifndef _Out_writes_bytes_opt_
> > +#define _Out_writes_bytes_opt_(s)
> > +#endif
> 
> It got added in da215fcf5f001d1fdedf82e2f1407e8ff0b6d453
> ('include/sal: Update sal definitions').

Right, but that's still not available on CentOS. Anyway, I am dropping
all the _*_ keywords from the definition below in followup version.

> 
> #ifndef protects it, ok
> 
> > +/* The following shoud be in cfgmgr32.h, but it isn't */
> > +#ifndef CM_Get_DevNode_Property
> > +CMAPI CONFIGRET WINAPI CM_Get_DevNode_PropertyW(
> > +    _In_  DEVINST               dnDevInst,
> > +    _In_  CONST DEVPROPKEY    * PropertyKey,
> > +    _Out_ DEVPROPTYPE         * PropertyType,
> > +    _Out_writes_bytes_opt_(*PropertyBufferSize) PBYTE PropertyBuffer,
> > +    _Inout_ PULONG              PropertyBufferSize,
> > +    _In_  ULONG                 ulFlags
> > +    );
> > +#define CM_Get_DevNode_Property                  CM_Get_DevNode_PropertyW
> > +#endif
> > +
> 
> #ifndef should protect it from future clash, ok
> 
> Did you open bugs for mingw64 updates?

It was stuck on my TODO list for some time, but yeah I already posted a
patch for that. Thanks for reminding me.

> 
> > +
> >  #ifndef SHTDN_REASON_FLAG_PLANNED
> >  #define SHTDN_REASON_FLAG_PLANNED 0x80000000
> >  #endif
> > @@ -2234,3 +2265,165 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> >
> >      return info;
> >  }
> > +
> > +/*
> > + * Safely get device property. Returned strings are using wide characters.
> > + * Caller is responsible for freeing the buffer.
> > + */
> > +static LPBYTE cm_get_property(DEVINST devInst, const DEVPROPKEY *propName,
> > +    PDEVPROPTYPE propType)
> > +{
> > +    CONFIGRET cr;
> > +    LPBYTE buffer = NULL;
> > +    ULONG bufferLen = 0;
> > +
> > +    /* First query for needed space */
> > +    cr = CM_Get_DevNode_Property(devInst, propName, propType,
> > +        buffer, &bufferLen, 0);
> 
> I think qemu-ga win32 code generally prefers to be explicit about A()
> vs W(), call the W function explicitely, remove the generic define.
> 
> > +    if ((cr != CR_SUCCESS) && (cr != CR_BUFFER_SMALL)) {
> 
> 
> This file is not a good example, but in general we avoid the extra
> parenthesis. Please drop them.
> 
> > +
> > +        g_debug(
> > +            "failed to get size of device property, device error code=%lx",
> > +            cr);
> 
> That file uses a mix of slog and g_debug..
> 
> I think slog() is higher level and preferable here. At least, try to
> make it fit on one line would be nice.
> 
> > +        return NULL;
> > +    }
> > +    buffer = (LPBYTE)g_malloc(bufferLen);
> 
> I'd suggest g_malloc0(len+1) to avoid surprises.
> 
> Drop the cast.
> 
> > +    cr = CM_Get_DevNode_Property(devInst, propName, propType,
> > +        buffer, &bufferLen, 0);
> > +    if (cr != CR_SUCCESS) {
> > +        g_free(buffer);
> 
> you could use g_auto declaration these days (I know, it wasn't there
> when you started, but now it is ;).
> 
> > +        g_debug(
> > +            "failed to get device property, device error code=%lx", cr);
> 
> same about slog
> 
> > +        return NULL;
> > +    }
> > +    return buffer;
> > +}
> > +
> > +/*
> > + * https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices
> > + */
> > +#define DEVICE_PCI_RE "PCI\\\\VEN_(1AF4|1B36)&DEV_([0-9A-B]{4})(&|$)"
> > +
> > +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> > +{
> > +    GuestDeviceInfoList *head = NULL, *cur_item = NULL, *item = NULL;
> > +    HDEVINFO dev_info = INVALID_HANDLE_VALUE;
> > +    SP_DEVINFO_DATA dev_info_data;
> > +    int i;
> > +    GError *gerr = NULL;
> > +    GRegex *device_pci_re = NULL;
> > +
> > +    device_pci_re = g_regex_new(DEVICE_PCI_RE,
> > +        G_REGEX_ANCHORED | G_REGEX_OPTIMIZE, 0,
> > +        &gerr);
> > +
> > +    if (gerr) {
> > +        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> > +        g_error_free(gerr);
> > +        goto out;
> > +    }
> 
> given that the rule is static, I think you could simplify error
> handling and use g_assert(device_pci_re != NULL) instead.
> 
> > +
> > +    dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> > +    dev_info = SetupDiGetClassDevs(0, 0, 0, DIGCF_PRESENT | DIGCF_ALLCLASSES);
> > +    if (dev_info == INVALID_HANDLE_VALUE) {
> > +        error_setg(errp, "failed to get device tree");
> > +        goto out;
> > +    }
> > +
> > +    g_debug("enumerating devices");
> > +    for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
> > +        LPWSTR name = NULL, hwIDs = NULL, lpValue;
> > +        bool skip = true;
> > +        DEVPROPTYPE cmType;
> > +        SYSTEMTIME stUTC;
> > +        LPFILETIME pfdDriverDate;
> > +        LPWSTR driverVersion;
> > +
> > +        GuestDeviceInfo *device = g_new0(GuestDeviceInfo, 1);
> > +
> > +        name = (LPWSTR)cm_get_property(dev_info_data.DevInst, &DEVPKEY_NAME,
> > +            &cmType);
> > +        if (name == NULL) {
> > +            g_debug("failed to get device description");
> > +            goto next;
> > +        }
> > +        device->driver_name = guest_wctomb_dup(name);
> 
> QMP strings should be utf8 encoded.
> 
> No idea why guest_wctomb_dup() was introduced in the first place to use ANSI CP.
> 
> Imho, every strings returned by W() functions should go through
> g_utf16_to_utf8().

Great! guest_wctomb_dup() seemed fishy to me from the start.

> 
> 
> > +        g_free(name);
> > +        g_debug("querying device: %s", device->driver_name);
> > +
> > +        hwIDs = (LPWSTR)cm_get_property(dev_info_data.DevInst,
> > +            &DEVPKEY_Device_HardwareIds, &cmType);
> > +        if (hwIDs == NULL) {
> > +            g_debug("failed to get hardware IDs");
> > +            goto next;
> > +        }
> > +        for (lpValue = hwIDs;
> > +            '\0' != *lpValue;
> > +            lpValue += lstrlenW(lpValue) + 1) {
> 
> ok (alternatively, introduce a function that returns a GStrv to make
> caller life easier.

I don't think I follow. Which part would you like to move into a
function?

> 
> > +            GMatchInfo *match_info;
> > +            char *hwID = guest_wctomb_dup(lpValue);
> > +            /* g_debug("hwID: %s", hwID); */
> > +            if (!g_regex_match(device_pci_re, hwID, 0, &match_info)) {
> > +                continue;
> 
> leaks hwID
> 
> > +            }
> > +            skip = false;
> > +            device->vendor_id = g_match_info_fetch(match_info, 1);
> > +            device->device_id = g_match_info_fetch(match_info, 2);
> > +            g_match_info_free(match_info);
> > +        }
> > +        free(hwIDs);
> 
> Let's stick to g_free() as you g_malloc()
> 
> > +
> > +        if (skip) {
> > +            goto next;
> > +        }
> > +
> > +        driverVersion = (LPWSTR)cm_get_property(dev_info_data.DevInst,
> > +            &DEVPKEY_Device_DriverVersion, &cmType);
> > +        if (driverVersion == NULL) {
> > +            g_debug("failed to get driver version");
> > +            goto next;
> > +        }
> > +        device->driver_version = guest_wctomb_dup(driverVersion);
> 
> I'd use g_utf16_to_utf8()
> 
> > +        free(driverVersion);
> 
> g_free()
> 
> > +
> > +        pfdDriverDate = (LPFILETIME)cm_get_property(dev_info_data.DevInst,
> > +            &DEVPKEY_Device_DriverDate, &cmType);
> > +        if (driverVersion == NULL) {
> > +            g_debug("failed to get driver date");
> > +            goto next;
> > +        }
> > +        FileTimeToSystemTime(pfdDriverDate, &stUTC);
> > +        free(pfdDriverDate);
> 
> g_free()
> 
> > +        device->driver_date = g_strdup_printf("%02d/%02d/%04d",
> > +            stUTC.wMonth, stUTC.wDay, stUTC.wYear);
> > +        g_debug("Driver Version: %s,%s\n", device->driver_date,
> > +            device->driver_version);
> > +
> > +        item = g_new0(GuestDeviceInfoList, 1);
> > +        item->value = device;
> > +        if (!cur_item) {
> > +            head = cur_item = item;
> > +        } else {
> > +            cur_item->next = item;
> > +            cur_item = item;
> > +        }
> > +        continue;
> > +
> > +next:
> > +        g_free(device->vendor_id);
> > +        g_free(device->device_id);
> > +        g_free(device->driver_date);
> > +        g_free(device->driver_name);
> > +        g_free(device->driver_version);
> > +        g_free(device);
> 
> Please switch to g_auto

Aye, aye, captain!

> 
> > +    }
> > +
> > +out:
> > +
> > +    if (dev_info != INVALID_HANDLE_VALUE) {
> > +        SetupDiDestroyDeviceInfoList(dev_info);
> > +    }
> > +    g_regex_unref(device_pci_re);
> > +
> > +    return head;
> > +}
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index fb4605cc19..ed73b0b1c6 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -1242,3 +1242,35 @@
> >  ##
> >  { 'command': 'guest-get-osinfo',
> >    'returns': 'GuestOSInfo' }
> > +
> > +##
> > +# @GuestDeviceInfo:
> > +#
> > +# @vendor-id: vendor ID as hexadecimal string in uper case without 0x prefix
> > +# @device-id: device ID as hexadecimal string in uper case without 0x prefix
> > +# @driver-name: name of the associated driver
> > +# @driver-date: driver release date in format MM/DD/YY
> > +# @driver-version: driver version
> > +#
> > +# Since: 4.1.1
> > +##
> > +{ 'struct': 'GuestDeviceInfo',
> > +  'data': {
> > +      'vendor-id': 'str',
> > +      'device-id': 'str',
> > +      'driver-name': 'str',
> > +      'driver-date': 'str',
> > +      'driver-version': 'str'
> > +      } }
> > +
> > +##
> > +# @guest-get-devices:
> > +#
> > +# Retrieve information about device drivers in Windows guest
> > +#
> > +# Returns: @GuestOSInfo
> 
> @GuestDeviceInfo
> 
> > +#
> > +# Since: 4.1.1
> > +##
> > +{ 'command': 'guest-get-devices',
> > +  'returns': ['GuestDeviceInfo'] }
> > --
> > 2.22.0
> >
> >
> 
> 
> -- 
> Marc-André Lureau

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


Re: [Qemu-devel] [PATCH] qga: add command guest-get-devices for reporting VirtIO devices
Posted by Marc-André Lureau 4 years, 7 months ago
Hi Tomáš

On Wed, Sep 18, 2019 at 3:23 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> On Mon, Sep 16, 2019 at 04:31:49PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Aug 29, 2019 at 8:06 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> > >
> > > Add command for reporting devices on Windows guest. The intent is not so
> > > much to report the devices but more importantly the driver (and its
> > > version) that is assigned to the device.
> > >
> > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > > ---
> > >  qga/commands-posix.c |  11 +++
> > >  qga/commands-win32.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
> > >  qga/qapi-schema.json |  32 +++++++
> > >  3 files changed, 237 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > index dfc05f5b8a..9adf8bb520 100644
> > > --- a/qga/commands-posix.c
> > > +++ b/qga/commands-posix.c
> > > @@ -2709,6 +2709,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
> > >
> > >      return 0;
> > >  }
> > > +
> > >  #endif /* CONFIG_FSFREEZE */
> >
> > extra line
> >
> >
> > >
> > >  #if !defined(CONFIG_FSTRIM)
> > > @@ -2757,6 +2758,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
> > >      blacklist = g_list_append(blacklist, g_strdup("guest-fstrim"));
> > >  #endif
> > >
> > > +    blacklist = g_list_append(blacklist, g_strdup("guest-get-devices"));
> > > +
> > >      return blacklist;
> > >  }
> > >
> > > @@ -2977,3 +2980,11 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> > >
> > >      return info;
> > >  }
> > > +
> > > +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> > > +{
> > > +    error_setg(errp, QERR_UNSUPPORTED);
> > > +
> > > +    return NULL;
> > > +}
> > > +
> >
> > extra EOF line
> >
> > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > > index 6b67f16faf..0bb93422c7 100644
> > > --- a/qga/commands-win32.c
> > > +++ b/qga/commands-win32.c
> > > @@ -21,10 +21,11 @@
> > >  #ifdef CONFIG_QGA_NTDDSCSI
> > >  #include <winioctl.h>
> > >  #include <ntddscsi.h>
> > > +#endif
> > >  #include <setupapi.h>
> > >  #include <cfgmgr32.h>
> > >  #include <initguid.h>
> > > -#endif
> > > +#include <devpropdef.h>
> > >  #include <lm.h>
> > >  #include <wtsapi32.h>
> > >  #include <wininet.h>
> > > @@ -38,6 +39,36 @@
> > >  #include "qemu/host-utils.h"
> > >  #include "qemu/base64.h"
> > >
> > > +
> > > +/* The following should be in devpkey.h, but it isn't */
> > > +DEFINE_DEVPROPKEY(DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5, 0xf1, 0x02,
> > > +    0x60, 0x8c, 0x9e, 0xeb, 0xac, 10);  /* DEVPROP_TYPE_STRING */
> > > +DEFINE_DEVPROPKEY(DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c, 0x4efd, 0x80,
> > > +    0x20, 0x67, 0xd1, 0x46, 0xa8, 0x50, 0xe0, 3);
> > > +    /* DEVPROP_TYPE_STRING_LIST */
> > > +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d, 0x4094, 0xad,
> > > +    0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 2);  /* DEVPROP_TYPE_FILETIME */
> > > +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverVersion, 0xa8b865dd, 0x2e3d, 0x4094,
> > > +    0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 3);
> >
> > perhaps use a qemu prefix to avoid future clash?
>
> Yes, this looks like the only option.
>
> >
> > > +    /* DEVPROP_TYPE_STRING */
> > > +/* The following should be in sal.h, but it isn't */
> > > +#ifndef _Out_writes_bytes_opt_
> > > +#define _Out_writes_bytes_opt_(s)
> > > +#endif
> >
> > It got added in da215fcf5f001d1fdedf82e2f1407e8ff0b6d453
> > ('include/sal: Update sal definitions').
>
> Right, but that's still not available on CentOS. Anyway, I am dropping
> all the _*_ keywords from the definition below in followup version.
>
> >
> > #ifndef protects it, ok
> >
> > > +/* The following shoud be in cfgmgr32.h, but it isn't */
> > > +#ifndef CM_Get_DevNode_Property
> > > +CMAPI CONFIGRET WINAPI CM_Get_DevNode_PropertyW(
> > > +    _In_  DEVINST               dnDevInst,
> > > +    _In_  CONST DEVPROPKEY    * PropertyKey,
> > > +    _Out_ DEVPROPTYPE         * PropertyType,
> > > +    _Out_writes_bytes_opt_(*PropertyBufferSize) PBYTE PropertyBuffer,
> > > +    _Inout_ PULONG              PropertyBufferSize,
> > > +    _In_  ULONG                 ulFlags
> > > +    );
> > > +#define CM_Get_DevNode_Property                  CM_Get_DevNode_PropertyW
> > > +#endif
> > > +
> >
> > #ifndef should protect it from future clash, ok
> >
> > Did you open bugs for mingw64 updates?
>
> It was stuck on my TODO list for some time, but yeah I already posted a
> patch for that. Thanks for reminding me.
>

nice

> >
> > > +
> > >  #ifndef SHTDN_REASON_FLAG_PLANNED
> > >  #define SHTDN_REASON_FLAG_PLANNED 0x80000000
> > >  #endif
> > > @@ -2234,3 +2265,165 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> > >
> > >      return info;
> > >  }
> > > +
> > > +/*
> > > + * Safely get device property. Returned strings are using wide characters.
> > > + * Caller is responsible for freeing the buffer.
> > > + */
> > > +static LPBYTE cm_get_property(DEVINST devInst, const DEVPROPKEY *propName,
> > > +    PDEVPROPTYPE propType)
> > > +{
> > > +    CONFIGRET cr;
> > > +    LPBYTE buffer = NULL;
> > > +    ULONG bufferLen = 0;
> > > +
> > > +    /* First query for needed space */
> > > +    cr = CM_Get_DevNode_Property(devInst, propName, propType,
> > > +        buffer, &bufferLen, 0);
> >
> > I think qemu-ga win32 code generally prefers to be explicit about A()
> > vs W(), call the W function explicitely, remove the generic define.
> >
> > > +    if ((cr != CR_SUCCESS) && (cr != CR_BUFFER_SMALL)) {
> >
> >
> > This file is not a good example, but in general we avoid the extra
> > parenthesis. Please drop them.
> >
> > > +
> > > +        g_debug(
> > > +            "failed to get size of device property, device error code=%lx",
> > > +            cr);
> >
> > That file uses a mix of slog and g_debug..
> >
> > I think slog() is higher level and preferable here. At least, try to
> > make it fit on one line would be nice.
> >
> > > +        return NULL;
> > > +    }
> > > +    buffer = (LPBYTE)g_malloc(bufferLen);
> >
> > I'd suggest g_malloc0(len+1) to avoid surprises.
> >
> > Drop the cast.
> >
> > > +    cr = CM_Get_DevNode_Property(devInst, propName, propType,
> > > +        buffer, &bufferLen, 0);
> > > +    if (cr != CR_SUCCESS) {
> > > +        g_free(buffer);
> >
> > you could use g_auto declaration these days (I know, it wasn't there
> > when you started, but now it is ;).
> >
> > > +        g_debug(
> > > +            "failed to get device property, device error code=%lx", cr);
> >
> > same about slog
> >
> > > +        return NULL;
> > > +    }
> > > +    return buffer;
> > > +}
> > > +
> > > +/*
> > > + * https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices
> > > + */
> > > +#define DEVICE_PCI_RE "PCI\\\\VEN_(1AF4|1B36)&DEV_([0-9A-B]{4})(&|$)"
> > > +
> > > +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> > > +{
> > > +    GuestDeviceInfoList *head = NULL, *cur_item = NULL, *item = NULL;
> > > +    HDEVINFO dev_info = INVALID_HANDLE_VALUE;
> > > +    SP_DEVINFO_DATA dev_info_data;
> > > +    int i;
> > > +    GError *gerr = NULL;
> > > +    GRegex *device_pci_re = NULL;
> > > +
> > > +    device_pci_re = g_regex_new(DEVICE_PCI_RE,
> > > +        G_REGEX_ANCHORED | G_REGEX_OPTIMIZE, 0,
> > > +        &gerr);
> > > +
> > > +    if (gerr) {
> > > +        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> > > +        g_error_free(gerr);
> > > +        goto out;
> > > +    }
> >
> > given that the rule is static, I think you could simplify error
> > handling and use g_assert(device_pci_re != NULL) instead.
> >
> > > +
> > > +    dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> > > +    dev_info = SetupDiGetClassDevs(0, 0, 0, DIGCF_PRESENT | DIGCF_ALLCLASSES);
> > > +    if (dev_info == INVALID_HANDLE_VALUE) {
> > > +        error_setg(errp, "failed to get device tree");
> > > +        goto out;
> > > +    }
> > > +
> > > +    g_debug("enumerating devices");
> > > +    for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
> > > +        LPWSTR name = NULL, hwIDs = NULL, lpValue;
> > > +        bool skip = true;
> > > +        DEVPROPTYPE cmType;
> > > +        SYSTEMTIME stUTC;
> > > +        LPFILETIME pfdDriverDate;
> > > +        LPWSTR driverVersion;
> > > +
> > > +        GuestDeviceInfo *device = g_new0(GuestDeviceInfo, 1);
> > > +
> > > +        name = (LPWSTR)cm_get_property(dev_info_data.DevInst, &DEVPKEY_NAME,
> > > +            &cmType);
> > > +        if (name == NULL) {
> > > +            g_debug("failed to get device description");
> > > +            goto next;
> > > +        }
> > > +        device->driver_name = guest_wctomb_dup(name);
> >
> > QMP strings should be utf8 encoded.
> >
> > No idea why guest_wctomb_dup() was introduced in the first place to use ANSI CP.
> >
> > Imho, every strings returned by W() functions should go through
> > g_utf16_to_utf8().
>
> Great! guest_wctomb_dup() seemed fishy to me from the start.
>
> >
> >
> > > +        g_free(name);
> > > +        g_debug("querying device: %s", device->driver_name);
> > > +
> > > +        hwIDs = (LPWSTR)cm_get_property(dev_info_data.DevInst,
> > > +            &DEVPKEY_Device_HardwareIds, &cmType);
> > > +        if (hwIDs == NULL) {
> > > +            g_debug("failed to get hardware IDs");
> > > +            goto next;
> > > +        }
> > > +        for (lpValue = hwIDs;
> > > +            '\0' != *lpValue;
> > > +            lpValue += lstrlenW(lpValue) + 1) {
> >
> > ok (alternatively, introduce a function that returns a GStrv to make
> > caller life easier.
>
> I don't think I follow. Which part would you like to move into a
> function?

I suggest to wrap "cm_get_property(dev_info_data.DevInst,
&DEVPKEY_Device_HardwareIds, &cmType);" in a function that returns a
GStrv of utf8 strings. This will seperate concerns and simplify caller
job, just an idea..


> >
> > > +            GMatchInfo *match_info;
> > > +            char *hwID = guest_wctomb_dup(lpValue);
> > > +            /* g_debug("hwID: %s", hwID); */
> > > +            if (!g_regex_match(device_pci_re, hwID, 0, &match_info)) {
> > > +                continue;
> >
> > leaks hwID
> >
> > > +            }
> > > +            skip = false;
> > > +            device->vendor_id = g_match_info_fetch(match_info, 1);
> > > +            device->device_id = g_match_info_fetch(match_info, 2);
> > > +            g_match_info_free(match_info);
> > > +        }
> > > +        free(hwIDs);
> >
> > Let's stick to g_free() as you g_malloc()
> >
> > > +
> > > +        if (skip) {
> > > +            goto next;
> > > +        }
> > > +
> > > +        driverVersion = (LPWSTR)cm_get_property(dev_info_data.DevInst,
> > > +            &DEVPKEY_Device_DriverVersion, &cmType);
> > > +        if (driverVersion == NULL) {
> > > +            g_debug("failed to get driver version");
> > > +            goto next;
> > > +        }
> > > +        device->driver_version = guest_wctomb_dup(driverVersion);
> >
> > I'd use g_utf16_to_utf8()
> >
> > > +        free(driverVersion);
> >
> > g_free()
> >
> > > +
> > > +        pfdDriverDate = (LPFILETIME)cm_get_property(dev_info_data.DevInst,
> > > +            &DEVPKEY_Device_DriverDate, &cmType);
> > > +        if (driverVersion == NULL) {
> > > +            g_debug("failed to get driver date");
> > > +            goto next;
> > > +        }
> > > +        FileTimeToSystemTime(pfdDriverDate, &stUTC);
> > > +        free(pfdDriverDate);
> >
> > g_free()
> >
> > > +        device->driver_date = g_strdup_printf("%02d/%02d/%04d",
> > > +            stUTC.wMonth, stUTC.wDay, stUTC.wYear);
> > > +        g_debug("Driver Version: %s,%s\n", device->driver_date,
> > > +            device->driver_version);
> > > +
> > > +        item = g_new0(GuestDeviceInfoList, 1);
> > > +        item->value = device;
> > > +        if (!cur_item) {
> > > +            head = cur_item = item;
> > > +        } else {
> > > +            cur_item->next = item;
> > > +            cur_item = item;
> > > +        }
> > > +        continue;
> > > +
> > > +next:
> > > +        g_free(device->vendor_id);
> > > +        g_free(device->device_id);
> > > +        g_free(device->driver_date);
> > > +        g_free(device->driver_name);
> > > +        g_free(device->driver_version);
> > > +        g_free(device);
> >
> > Please switch to g_auto
>
> Aye, aye, captain!
>
> >
> > > +    }
> > > +
> > > +out:
> > > +
> > > +    if (dev_info != INVALID_HANDLE_VALUE) {
> > > +        SetupDiDestroyDeviceInfoList(dev_info);
> > > +    }
> > > +    g_regex_unref(device_pci_re);
> > > +
> > > +    return head;
> > > +}
> > > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > > index fb4605cc19..ed73b0b1c6 100644
> > > --- a/qga/qapi-schema.json
> > > +++ b/qga/qapi-schema.json
> > > @@ -1242,3 +1242,35 @@
> > >  ##
> > >  { 'command': 'guest-get-osinfo',
> > >    'returns': 'GuestOSInfo' }
> > > +
> > > +##
> > > +# @GuestDeviceInfo:
> > > +#
> > > +# @vendor-id: vendor ID as hexadecimal string in uper case without 0x prefix
> > > +# @device-id: device ID as hexadecimal string in uper case without 0x prefix
> > > +# @driver-name: name of the associated driver
> > > +# @driver-date: driver release date in format MM/DD/YY
> > > +# @driver-version: driver version
> > > +#
> > > +# Since: 4.1.1
> > > +##
> > > +{ 'struct': 'GuestDeviceInfo',
> > > +  'data': {
> > > +      'vendor-id': 'str',
> > > +      'device-id': 'str',
> > > +      'driver-name': 'str',
> > > +      'driver-date': 'str',
> > > +      'driver-version': 'str'
> > > +      } }
> > > +
> > > +##
> > > +# @guest-get-devices:
> > > +#
> > > +# Retrieve information about device drivers in Windows guest
> > > +#
> > > +# Returns: @GuestOSInfo
> >
> > @GuestDeviceInfo
> >
> > > +#
> > > +# Since: 4.1.1
> > > +##
> > > +{ 'command': 'guest-get-devices',
> > > +  'returns': ['GuestDeviceInfo'] }
> > > --
> > > 2.22.0
> > >
> > >
> >
> >
> > --
> > Marc-André Lureau
>
> --
> Tomáš Golembiovský <tgolembi@redhat.com>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] qga: add command guest-get-devices for reporting VirtIO devices
Posted by Eric Blake 4 years, 7 months ago
On 8/29/19 11:03 AM, Tomáš Golembiovský wrote:
> Add command for reporting devices on Windows guest. The intent is not so
> much to report the devices but more importantly the driver (and its
> version) that is assigned to the device.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>

> +++ b/qga/qapi-schema.json
> @@ -1242,3 +1242,35 @@
>  ##
>  { 'command': 'guest-get-osinfo',
>    'returns': 'GuestOSInfo' }
> +
> +##
> +# @GuestDeviceInfo:
> +#
> +# @vendor-id: vendor ID as hexadecimal string in uper case without 0x prefix
> +# @device-id: device ID as hexadecimal string in uper case without 0x prefix

s/uper/upper/ twice

Should these be ints instead of strings (yes, it means they would be
decimal over the wire, which is not the typical representation)?

> +# @driver-name: name of the associated driver
> +# @driver-date: driver release date in format MM/DD/YY

Why US-centric?  Better would be something like ISO, YYYY-MM-DD

> +# @driver-version: driver version
> +#
> +# Since: 4.1.1

4.2.  We don't tend to add features on stable backport branches (as this
missed 4.1.0, we're unlikely to add it for 4.1.1).

> +##
> +{ 'struct': 'GuestDeviceInfo',
> +  'data': {
> +      'vendor-id': 'str',
> +      'device-id': 'str',
> +      'driver-name': 'str',
> +      'driver-date': 'str',
> +      'driver-version': 'str'
> +      } }
> +
> +##
> +# @guest-get-devices:
> +#
> +# Retrieve information about device drivers in Windows guest
> +#
> +# Returns: @GuestOSInfo
> +#
> +# Since: 4.1.1

again, 4.2

> +##
> +{ 'command': 'guest-get-devices',
> +  'returns': ['GuestDeviceInfo'] }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] qga: add command guest-get-devices for reporting VirtIO devices
Posted by Tomáš Golembiovský 4 years, 7 months ago
On Thu, 29 Aug 2019 11:13:53 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 8/29/19 11:03 AM, Tomáš Golembiovský wrote:
> > Add command for reporting devices on Windows guest. The intent is not so
> > much to report the devices but more importantly the driver (and its
> > version) that is assigned to the device.
> > 
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>  
> 
> > +++ b/qga/qapi-schema.json
> > @@ -1242,3 +1242,35 @@
> >  ##
> >  { 'command': 'guest-get-osinfo',
> >    'returns': 'GuestOSInfo' }
> > +
> > +##
> > +# @GuestDeviceInfo:
> > +#
> > +# @vendor-id: vendor ID as hexadecimal string in uper case without 0x prefix
> > +# @device-id: device ID as hexadecimal string in uper case without 0x prefix  
> 
> s/uper/upper/ twice

duh

> 
> Should these be ints instead of strings (yes, it means they would be
> decimal over the wire, which is not the typical representation)?

Yes, I'm also indecisive in this point too. Although I sort of planned
to switch it to `int` in v2 anyway.

> 
> > +# @driver-name: name of the associated driver
> > +# @driver-date: driver release date in format MM/DD/YY  
> 
> Why US-centric?  Better would be something like ISO, YYYY-MM-DD

I admit ISO form would be more fool proof (and I would like it better
too), but this is to stay consistent with format used in Windows
drivers[1].

> 
> > +# @driver-version: driver version
> > +#
> > +# Since: 4.1.1  
> 
> 4.2.  We don't tend to add features on stable backport branches (as this
> missed 4.1.0, we're unlikely to add it for 4.1.1).

Ah, ok.

Thanks,

    Tomas

[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/install/inf-driverver-directive

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