Re: [RFC 3/4] gqa-win: get_pci_info: Add g_autofree for few variables

[Philippe Mathieu-Daudé]

Hi Kostiantyn,

On 8/9/21 11:48 AM, Kostiantyn Kostiuk wrote:
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>

I'm not sure what you are trying to do here, fix a memory leak?

> ---
>  qga/commands-win32.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 724ce76a0e..a8a601776d 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -539,9 +539,9 @@ static GuestPCIAddress *get_pci_info(int number, Error 
> **errp)
>      dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
>      dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
>      for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
> -        PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
> +        g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = 
> NULL;
>          STORAGE_DEVICE_NUMBER sdn;
> -        char *parent_dev_id = NULL;
> +        g_autofree char *parent_dev_id = NULL;
>          HDEVINFO parent_dev_info;
>          SP_DEVINFO_DATA parent_dev_info_data;
>          DWORD j;
> 

Anyhow this function is confuse.

I think it would be easier to review by replacing the while()
by 2 calls, as suggested in the documentation:
https://docs.microsoft.com/en-us/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceinterfacedetaila

-- >8 --
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7bac0c5d422..2188c5dd80d 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -539,7 +539,6 @@ static GuestPCIAddress *get_pci_info(int number,
Error **errp)
     dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
     dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
     for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
-        PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
         STORAGE_DEVICE_NUMBER sdn;
         char *parent_dev_id = NULL;
         HDEVINFO parent_dev_info;
@@ -551,25 +550,36 @@ static GuestPCIAddress *get_pci_info(int number,
Error **errp)
         if (SetupDiEnumDeviceInterfaces(dev_info, &dev_info_data,
                                         &GUID_DEVINTERFACE_DISK, 0,
                                         &dev_iface_data)) {
-            while (!SetupDiGetDeviceInterfaceDetail(dev_info,
&dev_iface_data,
-                                                    pdev_iface_detail_data,
-                                                    size, &size,
-                                                    &dev_info_data)) {
-                if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
-                    pdev_iface_detail_data = g_malloc(size);
-                    pdev_iface_detail_data->cbSize =
-                        sizeof(*pdev_iface_detail_data);
-                } else {
-                    error_setg_win32(errp, GetLastError(),
-                                     "failed to get device interfaces");
-                    goto free_dev_info;
-                }
+            g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA
+                       pdev_iface_detail_data = NULL;
+
+            /* Get the required buffer size. */
+            if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
+                                                 NULL, 0, &size,
+                                                 &dev_info_data)
+                    && GetLastError() != ERROR_INSUFFICIENT_BUFFER) {
+                error_setg_win32(errp, GetLastError(),
+                                 "failed to get device interfaces
buffer size");
+                goto free_dev_info;
+            }
+
+            /* Allocate an appropriately sized buffer. */
+            pdev_iface_detail_data = g_malloc(size);
+            pdev_iface_detail_data->cbSize =
sizeof(*pdev_iface_detail_data);
+
+            /* Get the interface details. */
+            if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
+                                                 pdev_iface_detail_data,
+                                                 size, &size,
+                                                 &dev_info_data)) {
+                error_setg_win32(errp, GetLastError(),
+                                 "failed to get device interfaces");
+                goto free_dev_info;
             }

             dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
                                   FILE_SHARE_READ, NULL, OPEN_EXISTING, 0,
                                   NULL);
-            g_free(pdev_iface_detail_data);

             if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER,
                                  NULL, 0, &sdn, sizeof(sdn), &size,
NULL)) {
---