Re: [PATCH v3 3/3] qga: add implementation of guest-get-disks for Windows

[Tomáš Golembiovský]

On Wed, Oct 07, 2020 at 12:20:14PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Oct 7, 2020 at 11:46 AM Tomáš Golembiovský <tgolembi@redhat.com>
> wrote:
> 
> > The command lists all the physical disk drives. Unlike for Linux
> > partitions and virtual volumes are not listed.
> >
> > Example output:
> >
> > {
> >   "return": [
> >     {
> >       "name": "\\\\.\\PhysicalDrive0",
> >       "partition": false,
> >       "address": {
> >         "serial": "QM00001",
> >         "bus-type": "sata",
> >         ...
> >       },
> >       "dependents": []
> >     }
> >   ]
> > }
> >
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> >  qga/commands-win32.c | 98 +++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 92 insertions(+), 6 deletions(-)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 0dd16315d7..7fe5676785 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -979,6 +979,92 @@ out:
> >      return list;
> >  }
> >
> > +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> > +{
> >
> 
> ERRP_GUARD?

Is it necessary? We're not dereferencing errp anywhere.

> 
> +    GuestDiskInfoList *new = NULL, *ret = NULL;
> > +    HDEVINFO dev_info;
> > +    SP_DEVICE_INTERFACE_DATA dev_iface_data;
> > +    int i;
> > +
> > +    dev_info = SetupDiGetClassDevs(&GUID_DEVINTERFACE_DISK, 0, 0,
> > +        DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
> > +    if (dev_info == INVALID_HANDLE_VALUE) {
> > +        error_setg_win32(errp, GetLastError(), "failed to get device
> > tree");
> > +        return NULL;
> > +    }
> > +
> > +    g_debug("enumerating devices");
> > +    dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
> > +    for (i = 0;
> > +        SetupDiEnumDeviceInterfaces(dev_info, NULL,
> > &GUID_DEVINTERFACE_DISK,
> > +            i, &dev_iface_data);
> > +        i++) {
> > +        GuestDiskAddress *address = NULL;
> > +        GuestDiskInfo *disk = NULL;
> > +        Error *local_err = NULL;
> > +        g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA
> > +            pdev_iface_detail_data = NULL;
> > +        STORAGE_DEVICE_NUMBER sdn;
> > +        HANDLE dev_file;
> > +        DWORD size = 0;
> > +
> > +        g_debug("  getting device path");
> > +        while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
> > +                pdev_iface_detail_data,
> > +                size, &size,
> > +                NULL)) {
> > +            if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
> > +                pdev_iface_detail_data = g_realloc(pdev_iface_detail_data,
> > +                    size);
> > +                pdev_iface_detail_data->cbSize =
> > +                    sizeof(*pdev_iface_detail_data);
> > +            } else {
> > +                g_debug("failed to get device interface details");
> > +                continue;
> >
> 
> Is "continue" appropriate here? Looks like this may loop forever easily.

Duh! It would.

    Tomas

> 
> +            }
> > +        }
> > +
> > +        g_debug("  device: %s", pdev_iface_detail_data->DevicePath);
> > +        dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
> > +            FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL);
> > +        if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER,
> > +                NULL, 0, &sdn, sizeof(sdn), &size, NULL)) {
> > +            CloseHandle(dev_file);
> > +            debug_error("failed to get storage device number");
> > +            continue;
> > +        }
> > +        CloseHandle(dev_file);
> > +
> > +        disk = g_new0(GuestDiskInfo, 1);
> > +        disk->name = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
> > +            sdn.DeviceNumber);
> > +
> > +        g_debug("  number: %lu", sdn.DeviceNumber);
> > +        address = g_malloc0(sizeof(GuestDiskAddress));
> > +        address->has_dev = true;
> > +        address->dev = g_strdup(disk->name);
> > +        get_single_disk_info(sdn.DeviceNumber, address, &local_err);
> > +        if (local_err) {
> > +            g_debug("failed to get disk info: %s",
> > +                error_get_pretty(local_err));
> > +            error_free(local_err);
> > +            qapi_free_GuestDiskAddress(address);
> > +            address = NULL;
> > +        } else {
> > +            disk->address = address;
> > +            disk->has_address = true;
> > +        }
> > +
> > +        new = g_malloc0(sizeof(GuestDiskInfoList));
> > +        new->value = disk;
> > +        new->next = ret;
> > +        ret = new;
> > +    }
> > +
> > +    SetupDiDestroyDeviceInfoList(dev_info);
> > +    return ret;
> > +}
> > +
> >  #else
> >
> >  static GuestDiskAddressList *build_guest_disk_info(char *guid, Error
> > **errp)
> > @@ -986,6 +1072,12 @@ static GuestDiskAddressList
> > *build_guest_disk_info(char *guid, Error **errp)
> >      return NULL;
> >  }
> >
> > +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> > +{
> > +    error_setg(errp, QERR_UNSUPPORTED);
> > +    return NULL;
> > +}
> > +
> >  #endif /* CONFIG_QGA_NTDDSCSI */
> >
> >  static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
> > @@ -2457,9 +2549,3 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error
> > **errp)
> >      }
> >      return head;
> >  }
> > -
> > -GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> > -{
> > -    error_setg(errp, QERR_UNSUPPORTED);
> > -    return NULL;
> > -}
> > --
> > 2.28.0
> >
> >
> 
> -- 
> Marc-André Lureau

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