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

[Marc-André Lureau]

Hi Tomáš

On Wed, Sep 18, 2019 at 3:23 PM Tomáš Golembiovský <address@hidden> 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ý <address@hidden> 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ý <address@hidden>
> > > ---
> > >  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ý <address@hidden>
>


-- 
Marc-André Lureau