All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/6] gqa-win: get_pci_info: Fix memory leak
@ 2021-12-14 12:41 Kostiantyn Kostiuk
  2021-12-14 12:41 ` [PATCH v2 1/6] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-14 12:41 UTC (permalink / raw)
  To: qemu-devel, Michael Roth, Marc-André Lureau,
	Philippe Mathieu-Daudé

This set of patches fixes memory leaks in the get_pci_info function when it fails
to get parent device data. In this case the parent_dev_info and the parent_dev_id
variables will be initialized, but not freed.

Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1958825

Changelog:
v1 -> v2
 * Rename label "cleanup" -> "end"
 * Reorder some patches
 * Perform small refactoring: split logic to separate functions

Kostiantyn Kostiuk (6):
  gqa-win: get_pci_info: Clean dev_info if handle is valid
  gqa-win: get_pci_info: Use common 'end' label
  gqa-win: get_pci_info: Free parent_dev_info properly
  gqa-win: get_pci_info: Split logic to separate functions
  gqa-win: get_pci_info: Add g_autofree for few variables
  gqa-win: get_pci_info: Replace 'while' with 2 calls of the function

 qga/commands-win32.c | 235 ++++++++++++++++++++++++-------------------
 1 file changed, 134 insertions(+), 101 deletions(-)

--
2.25.1


^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH v2 1/6] gqa-win: get_pci_info: Clean dev_info if handle is valid
  2021-12-14 12:41 [PATCH v2 0/6] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
@ 2021-12-14 12:41 ` Kostiantyn Kostiuk
  2021-12-14 13:26   ` Marc-André Lureau
  2021-12-14 12:41 ` [PATCH v2 2/6] gqa-win: get_pci_info: Use common 'end' label Kostiantyn Kostiuk
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-14 12:41 UTC (permalink / raw)
  To: qemu-devel, Michael Roth, Marc-André Lureau,
	Philippe Mathieu-Daudé

Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
 qga/commands-win32.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 4e84afd83b..3dd74fe225 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -514,7 +514,7 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
 
 static GuestPCIAddress *get_pci_info(int number, Error **errp)
 {
-    HDEVINFO dev_info;
+    HDEVINFO dev_info = INVALID_HANDLE_VALUE;
     SP_DEVINFO_DATA dev_info_data;
     SP_DEVICE_INTERFACE_DATA dev_iface_data;
     HANDLE dev_file;
@@ -749,7 +749,9 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
     }
 
 free_dev_info:
-    SetupDiDestroyDeviceInfoList(dev_info);
+    if (dev_info != INVALID_HANDLE_VALUE) {
+        SetupDiDestroyDeviceInfoList(dev_info);
+    }
 out:
     return pci;
 }
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH v2 2/6] gqa-win: get_pci_info: Use common 'end' label
  2021-12-14 12:41 [PATCH v2 0/6] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
  2021-12-14 12:41 ` [PATCH v2 1/6] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk
@ 2021-12-14 12:41 ` Kostiantyn Kostiuk
  2021-12-14 13:26   ` Marc-André Lureau
  2021-12-14 12:41 ` [PATCH v2 3/6] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-14 12:41 UTC (permalink / raw)
  To: qemu-devel, Michael Roth, Marc-André Lureau,
	Philippe Mathieu-Daudé

To prevent memory leaks, always try to free initialized variables.

Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
 qga/commands-win32.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3dd74fe225..93c5375132 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -532,7 +532,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
                                    DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
     if (dev_info == INVALID_HANDLE_VALUE) {
         error_setg_win32(errp, GetLastError(), "failed to get devices tree");
-        goto out;
+        goto end;
     }
 
     g_debug("enumerating devices");
@@ -562,7 +562,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
                 } else {
                     error_setg_win32(errp, GetLastError(),
                                      "failed to get device interfaces");
-                    goto free_dev_info;
+                    goto end;
                 }
             }
 
@@ -576,7 +576,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
                 CloseHandle(dev_file);
                 error_setg_win32(errp, GetLastError(),
                                  "failed to get device slot number");
-                goto free_dev_info;
+                goto end;
             }
 
             CloseHandle(dev_file);
@@ -586,7 +586,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
         } else {
             error_setg_win32(errp, GetLastError(),
                              "failed to get device interfaces");
-            goto free_dev_info;
+            goto end;
         }
 
         g_debug("found device slot %d. Getting storage controller", number);
@@ -603,7 +603,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
                 } else {
                     error_setg_win32(errp, GetLastError(),
                                      "failed to get device instance ID");
-                    goto out;
+                    goto end;
                 }
             }
 
@@ -617,14 +617,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
                 g_error("CM_Locate_DevInst failed with code %lx", cr);
                 error_setg_win32(errp, GetLastError(),
                                  "failed to get device instance");
-                goto out;
+                goto end;
             }
             cr = CM_Get_Parent(&parent_dev_inst, dev_inst, 0);
             if (cr != CR_SUCCESS) {
                 g_error("CM_Get_Parent failed with code %lx", cr);
                 error_setg_win32(errp, GetLastError(),
                                  "failed to get parent device instance");
-                goto out;
+                goto end;
             }
 
             cr = CM_Get_Device_ID_Size(&dev_id_size, parent_dev_inst, 0);
@@ -632,7 +632,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
                 g_error("CM_Get_Device_ID_Size failed with code %lx", cr);
                 error_setg_win32(errp, GetLastError(),
                                  "failed to get parent device ID length");
-                goto out;
+                goto end;
             }
 
             ++dev_id_size;
@@ -647,7 +647,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
                 g_error("CM_Get_Device_ID failed with code %lx", cr);
                 error_setg_win32(errp, GetLastError(),
                                  "failed to get parent device ID");
-                goto out;
+                goto end;
             }
         }
 
@@ -661,14 +661,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
         if (parent_dev_info == INVALID_HANDLE_VALUE) {
             error_setg_win32(errp, GetLastError(),
                              "failed to get parent device");
-            goto out;
+            goto end;
         }
 
         parent_dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
         if (!SetupDiEnumDeviceInfo(parent_dev_info, 0, &parent_dev_info_data)) {
             error_setg_win32(errp, GetLastError(),
                            "failed to get parent device data");
-            goto out;
+            goto end;
         }
 
         for (j = 0;
@@ -748,11 +748,10 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
         break;
     }
 
-free_dev_info:
+end:
     if (dev_info != INVALID_HANDLE_VALUE) {
         SetupDiDestroyDeviceInfoList(dev_info);
     }
-out:
     return pci;
 }
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH v2 3/6] gqa-win: get_pci_info: Free parent_dev_info properly
  2021-12-14 12:41 [PATCH v2 0/6] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
  2021-12-14 12:41 ` [PATCH v2 1/6] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk
  2021-12-14 12:41 ` [PATCH v2 2/6] gqa-win: get_pci_info: Use common 'end' label Kostiantyn Kostiuk
@ 2021-12-14 12:41 ` Kostiantyn Kostiuk
  2021-12-14 13:26   ` Marc-André Lureau
  2021-12-14 12:41 ` [PATCH v2 4/6] gqa-win: get_pci_info: Split logic to separate functions Kostiantyn Kostiuk
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-14 12:41 UTC (permalink / raw)
  To: qemu-devel, Michael Roth, Marc-André Lureau,
	Philippe Mathieu-Daudé

In case when the function fails to get parent device data,
the parent_dev_info variable will be initialized, but not freed.

Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
 qga/commands-win32.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 93c5375132..f6de9e2676 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -515,6 +515,8 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
 static GuestPCIAddress *get_pci_info(int number, Error **errp)
 {
     HDEVINFO dev_info = INVALID_HANDLE_VALUE;
+    HDEVINFO parent_dev_info = INVALID_HANDLE_VALUE;
+
     SP_DEVINFO_DATA dev_info_data;
     SP_DEVICE_INTERFACE_DATA dev_iface_data;
     HANDLE dev_file;
@@ -542,7 +544,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
         PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
         STORAGE_DEVICE_NUMBER sdn;
         char *parent_dev_id = NULL;
-        HDEVINFO parent_dev_info;
         SP_DEVINFO_DATA parent_dev_info_data;
         DWORD j;
         DWORD size = 0;
@@ -744,11 +745,13 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
                 break;
             }
         }
-        SetupDiDestroyDeviceInfoList(parent_dev_info);
         break;
     }
 
 end:
+    if (parent_dev_info != INVALID_HANDLE_VALUE) {
+        SetupDiDestroyDeviceInfoList(parent_dev_info);
+    }
     if (dev_info != INVALID_HANDLE_VALUE) {
         SetupDiDestroyDeviceInfoList(dev_info);
     }
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH v2 4/6] gqa-win: get_pci_info: Split logic to separate functions
  2021-12-14 12:41 [PATCH v2 0/6] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
                   ` (2 preceding siblings ...)
  2021-12-14 12:41 ` [PATCH v2 3/6] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk
@ 2021-12-14 12:41 ` Kostiantyn Kostiuk
  2021-12-14 13:27   ` Marc-André Lureau
  2021-12-14 12:41 ` [PATCH v2 5/6] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk
  2021-12-14 12:41 ` [PATCH v2 6/6] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function Kostiantyn Kostiuk
  5 siblings, 1 reply; 13+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-14 12:41 UTC (permalink / raw)
  To: qemu-devel, Michael Roth, Marc-André Lureau,
	Philippe Mathieu-Daudé

Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
 qga/commands-win32.c | 161 +++++++++++++++++++++++--------------------
 1 file changed, 87 insertions(+), 74 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index f6de9e2676..8588fa8633 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -512,6 +512,92 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
         0x2accfe60L, 0xc130, 0x11d2, 0xb0, 0x82,
         0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b);

+static void get_pci_address_for_device(GuestPCIAddress *pci,
+                                       HDEVINFO dev_info)
+{
+    SP_DEVINFO_DATA dev_info_data;
+    DWORD j;
+    DWORD size;
+    bool partial_pci = false;
+
+    dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
+
+    for (j = 0;
+         SetupDiEnumDeviceInfo(dev_info, j, &dev_info_data);
+         j++) {
+        DWORD addr, bus, ui_slot, type;
+        int func, slot;
+        size = sizeof(DWORD);
+
+        /*
+        * There is no need to allocate buffer in the next functions. The
+        * size is known and ULONG according to
+        * https://msdn.microsoft.com/en-us/library/windows/hardware/ff543095(v=vs.85).aspx
+        */
+        if (!SetupDiGetDeviceRegistryProperty(
+                dev_info, &dev_info_data, SPDRP_BUSNUMBER,
+                &type, (PBYTE)&bus, size, NULL)) {
+            debug_error("failed to get PCI bus");
+            bus = -1;
+            partial_pci = true;
+        }
+
+        /*
+        * The function retrieves the device's address. This value will be
+        * transformed into device function and number
+        */
+        if (!SetupDiGetDeviceRegistryProperty(
+                dev_info, &dev_info_data, SPDRP_ADDRESS,
+                &type, (PBYTE)&addr, size, NULL)) {
+            debug_error("failed to get PCI address");
+            addr = -1;
+            partial_pci = true;
+        }
+
+        /*
+        * This call returns UINumber of DEVICE_CAPABILITIES structure.
+        * This number is typically a user-perceived slot number.
+        */
+        if (!SetupDiGetDeviceRegistryProperty(
+                dev_info, &dev_info_data, SPDRP_UI_NUMBER,
+                &type, (PBYTE)&ui_slot, size, NULL)) {
+            debug_error("failed to get PCI slot");
+            ui_slot = -1;
+            partial_pci = true;
+        }
+
+        /*
+        * SetupApi gives us the same information as driver with
+        * IoGetDeviceProperty. According to Microsoft:
+        *
+        *   FunctionNumber = (USHORT)((propertyAddress) & 0x0000FFFF)
+        *   DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF)
+        *   SPDRP_ADDRESS is propertyAddress, so we do the same.
+        *
+        * https://docs.microsoft.com/en-us/windows/desktop/api/setupapi/nf-setupapi-setupdigetdeviceregistrypropertya
+        */
+        if (partial_pci) {
+            pci->domain = -1;
+            pci->slot = -1;
+            pci->function = -1;
+            pci->bus = -1;
+            continue;
+        } else {
+            func = ((int)addr == -1) ? -1 : addr & 0x0000FFFF;
+            slot = ((int)addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF;
+            if ((int)ui_slot != slot) {
+                g_debug("mismatch with reported slot values: %d vs %d",
+                        (int)ui_slot, slot);
+            }
+            pci->domain = 0;
+            pci->slot = (int)ui_slot;
+            pci->function = func;
+            pci->bus = (int)bus;
+            return;
+        }
+    }
+}
+
 static GuestPCIAddress *get_pci_info(int number, Error **errp)
 {
     HDEVINFO dev_info = INVALID_HANDLE_VALUE;
@@ -522,7 +608,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
     HANDLE dev_file;
     int i;
     GuestPCIAddress *pci = NULL;
-    bool partial_pci = false;

     pci = g_malloc0(sizeof(*pci));
     pci->domain = -1;
@@ -545,7 +630,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
         STORAGE_DEVICE_NUMBER sdn;
         char *parent_dev_id = NULL;
         SP_DEVINFO_DATA parent_dev_info_data;
-        DWORD j;
         DWORD size = 0;

         g_debug("getting device path");
@@ -672,79 +756,8 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
             goto end;
         }

-        for (j = 0;
-             SetupDiEnumDeviceInfo(parent_dev_info, j, &parent_dev_info_data);
-             j++) {
-            DWORD addr, bus, ui_slot, type;
-            int func, slot;
+        get_pci_address_for_device(pci, parent_dev_info);

-            /*
-             * There is no need to allocate buffer in the next functions. The
-             * size is known and ULONG according to
-             * https://msdn.microsoft.com/en-us/library/windows/hardware/ff543095(v=vs.85).aspx
-             */
-            if (!SetupDiGetDeviceRegistryProperty(
-                  parent_dev_info, &parent_dev_info_data, SPDRP_BUSNUMBER,
-                  &type, (PBYTE)&bus, size, NULL)) {
-                debug_error("failed to get PCI bus");
-                bus = -1;
-                partial_pci = true;
-            }
-
-            /*
-             * The function retrieves the device's address. This value will be
-             * transformed into device function and number
-             */
-            if (!SetupDiGetDeviceRegistryProperty(
-                    parent_dev_info, &parent_dev_info_data, SPDRP_ADDRESS,
-                    &type, (PBYTE)&addr, size, NULL)) {
-                debug_error("failed to get PCI address");
-                addr = -1;
-                partial_pci = true;
-            }
-
-            /*
-             * This call returns UINumber of DEVICE_CAPABILITIES structure.
-             * This number is typically a user-perceived slot number.
-             */
-            if (!SetupDiGetDeviceRegistryProperty(
-                    parent_dev_info, &parent_dev_info_data, SPDRP_UI_NUMBER,
-                    &type, (PBYTE)&ui_slot, size, NULL)) {
-                debug_error("failed to get PCI slot");
-                ui_slot = -1;
-                partial_pci = true;
-            }
-
-            /*
-             * SetupApi gives us the same information as driver with
-             * IoGetDeviceProperty. According to Microsoft:
-             *
-             *   FunctionNumber = (USHORT)((propertyAddress) & 0x0000FFFF)
-             *   DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF)
-             *   SPDRP_ADDRESS is propertyAddress, so we do the same.
-             *
-             * https://docs.microsoft.com/en-us/windows/desktop/api/setupapi/nf-setupapi-setupdigetdeviceregistrypropertya
-             */
-            if (partial_pci) {
-                pci->domain = -1;
-                pci->slot = -1;
-                pci->function = -1;
-                pci->bus = -1;
-                continue;
-            } else {
-                func = ((int)addr == -1) ? -1 : addr & 0x0000FFFF;
-                slot = ((int)addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF;
-                if ((int)ui_slot != slot) {
-                    g_debug("mismatch with reported slot values: %d vs %d",
-                            (int)ui_slot, slot);
-                }
-                pci->domain = 0;
-                pci->slot = (int)ui_slot;
-                pci->function = func;
-                pci->bus = (int)bus;
-                break;
-            }
-        }
         break;
     }

--
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH v2 5/6] gqa-win: get_pci_info: Add g_autofree for few variables
  2021-12-14 12:41 [PATCH v2 0/6] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
                   ` (3 preceding siblings ...)
  2021-12-14 12:41 ` [PATCH v2 4/6] gqa-win: get_pci_info: Split logic to separate functions Kostiantyn Kostiuk
@ 2021-12-14 12:41 ` Kostiantyn Kostiuk
  2021-12-14 13:28   ` Marc-André Lureau
  2021-12-14 12:41 ` [PATCH v2 6/6] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function Kostiantyn Kostiuk
  5 siblings, 1 reply; 13+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-14 12:41 UTC (permalink / raw)
  To: qemu-devel, Michael Roth, Marc-André Lureau,
	Philippe Mathieu-Daudé

Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
 qga/commands-win32.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 8588fa8633..3092566313 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -626,9 +626,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;
         SP_DEVINFO_DATA parent_dev_info_data;
         DWORD size = 0;
 
@@ -654,7 +654,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
             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)) {
@@ -741,7 +740,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
         parent_dev_info =
             SetupDiGetClassDevs(&GUID_DEVINTERFACE_STORAGEPORT, parent_dev_id,
                                 NULL, DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
-        g_free(parent_dev_id);
 
         if (parent_dev_info == INVALID_HANDLE_VALUE) {
             error_setg_win32(errp, GetLastError(),
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH v2 6/6] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function
  2021-12-14 12:41 [PATCH v2 0/6] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
                   ` (4 preceding siblings ...)
  2021-12-14 12:41 ` [PATCH v2 5/6] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk
@ 2021-12-14 12:41 ` Kostiantyn Kostiuk
  2021-12-14 13:28   ` Marc-André Lureau
  5 siblings, 1 reply; 13+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-14 12:41 UTC (permalink / raw)
  To: qemu-devel, Michael Roth, Marc-André Lureau,
	Philippe Mathieu-Daudé

Microsoft suggests this solution in the documentation:
https://docs.microsoft.com/en-us/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceinterfacedetaila

Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
 qga/commands-win32.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3092566313..892082504f 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -636,10 +636,10 @@ 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 (!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 =
@@ -651,6 +651,16 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
                 }
             }
 
+            if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
+                                                 pdev_iface_detail_data,
+                                                 size, &size,
+                                                 &dev_info_data)) {
+                // pdev_iface_detail_data already is allocated
+                error_setg_win32(errp, GetLastError(),
+                                    "failed to get device interfaces");
+                goto end;
+            }
+
             dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
                                   FILE_SHARE_READ, NULL, OPEN_EXISTING, 0,
                                   NULL);
@@ -680,8 +690,8 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
             ULONG dev_id_size = 0;
 
             size = 0;
-            while (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
-                                               parent_dev_id, size, &size)) {
+            if (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
+                                            parent_dev_id, size, &size)) {
                 if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
                     parent_dev_id = g_malloc(size);
                 } else {
@@ -691,6 +701,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
                 }
             }
 
+            if (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
+                                            parent_dev_id, size, &size)) {
+                // parent_dev_id already is allocated
+                error_setg_win32(errp, GetLastError(),
+                                    "failed to get device instance ID");
+                goto end;
+            }
+
             /*
              * CM API used here as opposed to
              * SetupDiGetDeviceProperty(..., DEVPKEY_Device_Parent, ...)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/6] gqa-win: get_pci_info: Clean dev_info if handle is valid
  2021-12-14 12:41 ` [PATCH v2 1/6] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk
@ 2021-12-14 13:26   ` Marc-André Lureau
  0 siblings, 0 replies; 13+ messages in thread
From: Marc-André Lureau @ 2021-12-14 13:26 UTC (permalink / raw)
  To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel

On Tue, Dec 14, 2021 at 4:41 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  qga/commands-win32.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 4e84afd83b..3dd74fe225 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -514,7 +514,7 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
>
>  static GuestPCIAddress *get_pci_info(int number, Error **errp)
>  {
> -    HDEVINFO dev_info;
> +    HDEVINFO dev_info = INVALID_HANDLE_VALUE;
>      SP_DEVINFO_DATA dev_info_data;
>      SP_DEVICE_INTERFACE_DATA dev_iface_data;
>      HANDLE dev_file;
> @@ -749,7 +749,9 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>      }
>
>  free_dev_info:
> -    SetupDiDestroyDeviceInfoList(dev_info);
> +    if (dev_info != INVALID_HANDLE_VALUE) {
> +        SetupDiDestroyDeviceInfoList(dev_info);
> +    }
>  out:
>      return pci;
>  }
> --
> 2.25.1
>



^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/6] gqa-win: get_pci_info: Use common 'end' label
  2021-12-14 12:41 ` [PATCH v2 2/6] gqa-win: get_pci_info: Use common 'end' label Kostiantyn Kostiuk
@ 2021-12-14 13:26   ` Marc-André Lureau
  0 siblings, 0 replies; 13+ messages in thread
From: Marc-André Lureau @ 2021-12-14 13:26 UTC (permalink / raw)
  To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel

On Tue, Dec 14, 2021 at 4:41 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> To prevent memory leaks, always try to free initialized variables.
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  qga/commands-win32.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3dd74fe225..93c5375132 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -532,7 +532,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>                                     DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
>      if (dev_info == INVALID_HANDLE_VALUE) {
>          error_setg_win32(errp, GetLastError(), "failed to get devices tree");
> -        goto out;
> +        goto end;
>      }
>
>      g_debug("enumerating devices");
> @@ -562,7 +562,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>                  } else {
>                      error_setg_win32(errp, GetLastError(),
>                                       "failed to get device interfaces");
> -                    goto free_dev_info;
> +                    goto end;
>                  }
>              }
>
> @@ -576,7 +576,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>                  CloseHandle(dev_file);
>                  error_setg_win32(errp, GetLastError(),
>                                   "failed to get device slot number");
> -                goto free_dev_info;
> +                goto end;
>              }
>
>              CloseHandle(dev_file);
> @@ -586,7 +586,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>          } else {
>              error_setg_win32(errp, GetLastError(),
>                               "failed to get device interfaces");
> -            goto free_dev_info;
> +            goto end;
>          }
>
>          g_debug("found device slot %d. Getting storage controller", number);
> @@ -603,7 +603,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>                  } else {
>                      error_setg_win32(errp, GetLastError(),
>                                       "failed to get device instance ID");
> -                    goto out;
> +                    goto end;
>                  }
>              }
>
> @@ -617,14 +617,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>                  g_error("CM_Locate_DevInst failed with code %lx", cr);
>                  error_setg_win32(errp, GetLastError(),
>                                   "failed to get device instance");
> -                goto out;
> +                goto end;
>              }
>              cr = CM_Get_Parent(&parent_dev_inst, dev_inst, 0);
>              if (cr != CR_SUCCESS) {
>                  g_error("CM_Get_Parent failed with code %lx", cr);
>                  error_setg_win32(errp, GetLastError(),
>                                   "failed to get parent device instance");
> -                goto out;
> +                goto end;
>              }
>
>              cr = CM_Get_Device_ID_Size(&dev_id_size, parent_dev_inst, 0);
> @@ -632,7 +632,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>                  g_error("CM_Get_Device_ID_Size failed with code %lx", cr);
>                  error_setg_win32(errp, GetLastError(),
>                                   "failed to get parent device ID length");
> -                goto out;
> +                goto end;
>              }
>
>              ++dev_id_size;
> @@ -647,7 +647,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>                  g_error("CM_Get_Device_ID failed with code %lx", cr);
>                  error_setg_win32(errp, GetLastError(),
>                                   "failed to get parent device ID");
> -                goto out;
> +                goto end;
>              }
>          }
>
> @@ -661,14 +661,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>          if (parent_dev_info == INVALID_HANDLE_VALUE) {
>              error_setg_win32(errp, GetLastError(),
>                               "failed to get parent device");
> -            goto out;
> +            goto end;
>          }
>
>          parent_dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
>          if (!SetupDiEnumDeviceInfo(parent_dev_info, 0, &parent_dev_info_data)) {
>              error_setg_win32(errp, GetLastError(),
>                             "failed to get parent device data");
> -            goto out;
> +            goto end;
>          }
>
>          for (j = 0;
> @@ -748,11 +748,10 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>          break;
>      }
>
> -free_dev_info:
> +end:
>      if (dev_info != INVALID_HANDLE_VALUE) {
>          SetupDiDestroyDeviceInfoList(dev_info);
>      }
> -out:
>      return pci;
>  }
>
> --
> 2.25.1
>



^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/6] gqa-win: get_pci_info: Free parent_dev_info properly
  2021-12-14 12:41 ` [PATCH v2 3/6] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk
@ 2021-12-14 13:26   ` Marc-André Lureau
  0 siblings, 0 replies; 13+ messages in thread
From: Marc-André Lureau @ 2021-12-14 13:26 UTC (permalink / raw)
  To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel

On Tue, Dec 14, 2021 at 4:41 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> In case when the function fails to get parent device data,
> the parent_dev_info variable will be initialized, but not freed.
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  qga/commands-win32.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 93c5375132..f6de9e2676 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -515,6 +515,8 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
>  static GuestPCIAddress *get_pci_info(int number, Error **errp)
>  {
>      HDEVINFO dev_info = INVALID_HANDLE_VALUE;
> +    HDEVINFO parent_dev_info = INVALID_HANDLE_VALUE;
> +
>      SP_DEVINFO_DATA dev_info_data;
>      SP_DEVICE_INTERFACE_DATA dev_iface_data;
>      HANDLE dev_file;
> @@ -542,7 +544,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>          PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
>          STORAGE_DEVICE_NUMBER sdn;
>          char *parent_dev_id = NULL;
> -        HDEVINFO parent_dev_info;
>          SP_DEVINFO_DATA parent_dev_info_data;
>          DWORD j;
>          DWORD size = 0;
> @@ -744,11 +745,13 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>                  break;
>              }
>          }
> -        SetupDiDestroyDeviceInfoList(parent_dev_info);
>          break;
>      }
>
>  end:
> +    if (parent_dev_info != INVALID_HANDLE_VALUE) {
> +        SetupDiDestroyDeviceInfoList(parent_dev_info);
> +    }
>      if (dev_info != INVALID_HANDLE_VALUE) {
>          SetupDiDestroyDeviceInfoList(dev_info);
>      }
> --
> 2.25.1
>



^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/6] gqa-win: get_pci_info: Split logic to separate functions
  2021-12-14 12:41 ` [PATCH v2 4/6] gqa-win: get_pci_info: Split logic to separate functions Kostiantyn Kostiuk
@ 2021-12-14 13:27   ` Marc-André Lureau
  0 siblings, 0 replies; 13+ messages in thread
From: Marc-André Lureau @ 2021-12-14 13:27 UTC (permalink / raw)
  To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel

On Tue, Dec 14, 2021 at 4:41 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

thanks!

> ---
>  qga/commands-win32.c | 161 +++++++++++++++++++++++--------------------
>  1 file changed, 87 insertions(+), 74 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index f6de9e2676..8588fa8633 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -512,6 +512,92 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
>          0x2accfe60L, 0xc130, 0x11d2, 0xb0, 0x82,
>          0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b);
>
> +static void get_pci_address_for_device(GuestPCIAddress *pci,
> +                                       HDEVINFO dev_info)
> +{
> +    SP_DEVINFO_DATA dev_info_data;
> +    DWORD j;
> +    DWORD size;
> +    bool partial_pci = false;
> +
> +    dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> +
> +    for (j = 0;
> +         SetupDiEnumDeviceInfo(dev_info, j, &dev_info_data);
> +         j++) {
> +        DWORD addr, bus, ui_slot, type;
> +        int func, slot;
> +        size = sizeof(DWORD);
> +
> +        /*
> +        * There is no need to allocate buffer in the next functions. The
> +        * size is known and ULONG according to
> +        * https://msdn.microsoft.com/en-us/library/windows/hardware/ff543095(v=vs.85).aspx
> +        */
> +        if (!SetupDiGetDeviceRegistryProperty(
> +                dev_info, &dev_info_data, SPDRP_BUSNUMBER,
> +                &type, (PBYTE)&bus, size, NULL)) {
> +            debug_error("failed to get PCI bus");
> +            bus = -1;
> +            partial_pci = true;
> +        }
> +
> +        /*
> +        * The function retrieves the device's address. This value will be
> +        * transformed into device function and number
> +        */
> +        if (!SetupDiGetDeviceRegistryProperty(
> +                dev_info, &dev_info_data, SPDRP_ADDRESS,
> +                &type, (PBYTE)&addr, size, NULL)) {
> +            debug_error("failed to get PCI address");
> +            addr = -1;
> +            partial_pci = true;
> +        }
> +
> +        /*
> +        * This call returns UINumber of DEVICE_CAPABILITIES structure.
> +        * This number is typically a user-perceived slot number.
> +        */
> +        if (!SetupDiGetDeviceRegistryProperty(
> +                dev_info, &dev_info_data, SPDRP_UI_NUMBER,
> +                &type, (PBYTE)&ui_slot, size, NULL)) {
> +            debug_error("failed to get PCI slot");
> +            ui_slot = -1;
> +            partial_pci = true;
> +        }
> +
> +        /*
> +        * SetupApi gives us the same information as driver with
> +        * IoGetDeviceProperty. According to Microsoft:
> +        *
> +        *   FunctionNumber = (USHORT)((propertyAddress) & 0x0000FFFF)
> +        *   DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF)
> +        *   SPDRP_ADDRESS is propertyAddress, so we do the same.
> +        *
> +        * https://docs.microsoft.com/en-us/windows/desktop/api/setupapi/nf-setupapi-setupdigetdeviceregistrypropertya
> +        */
> +        if (partial_pci) {
> +            pci->domain = -1;
> +            pci->slot = -1;
> +            pci->function = -1;
> +            pci->bus = -1;
> +            continue;
> +        } else {
> +            func = ((int)addr == -1) ? -1 : addr & 0x0000FFFF;
> +            slot = ((int)addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF;
> +            if ((int)ui_slot != slot) {
> +                g_debug("mismatch with reported slot values: %d vs %d",
> +                        (int)ui_slot, slot);
> +            }
> +            pci->domain = 0;
> +            pci->slot = (int)ui_slot;
> +            pci->function = func;
> +            pci->bus = (int)bus;
> +            return;
> +        }
> +    }
> +}
> +
>  static GuestPCIAddress *get_pci_info(int number, Error **errp)
>  {
>      HDEVINFO dev_info = INVALID_HANDLE_VALUE;
> @@ -522,7 +608,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>      HANDLE dev_file;
>      int i;
>      GuestPCIAddress *pci = NULL;
> -    bool partial_pci = false;
>
>      pci = g_malloc0(sizeof(*pci));
>      pci->domain = -1;
> @@ -545,7 +630,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>          STORAGE_DEVICE_NUMBER sdn;
>          char *parent_dev_id = NULL;
>          SP_DEVINFO_DATA parent_dev_info_data;
> -        DWORD j;
>          DWORD size = 0;
>
>          g_debug("getting device path");
> @@ -672,79 +756,8 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>              goto end;
>          }
>
> -        for (j = 0;
> -             SetupDiEnumDeviceInfo(parent_dev_info, j, &parent_dev_info_data);
> -             j++) {
> -            DWORD addr, bus, ui_slot, type;
> -            int func, slot;
> +        get_pci_address_for_device(pci, parent_dev_info);
>
> -            /*
> -             * There is no need to allocate buffer in the next functions. The
> -             * size is known and ULONG according to
> -             * https://msdn.microsoft.com/en-us/library/windows/hardware/ff543095(v=vs.85).aspx
> -             */
> -            if (!SetupDiGetDeviceRegistryProperty(
> -                  parent_dev_info, &parent_dev_info_data, SPDRP_BUSNUMBER,
> -                  &type, (PBYTE)&bus, size, NULL)) {
> -                debug_error("failed to get PCI bus");
> -                bus = -1;
> -                partial_pci = true;
> -            }
> -
> -            /*
> -             * The function retrieves the device's address. This value will be
> -             * transformed into device function and number
> -             */
> -            if (!SetupDiGetDeviceRegistryProperty(
> -                    parent_dev_info, &parent_dev_info_data, SPDRP_ADDRESS,
> -                    &type, (PBYTE)&addr, size, NULL)) {
> -                debug_error("failed to get PCI address");
> -                addr = -1;
> -                partial_pci = true;
> -            }
> -
> -            /*
> -             * This call returns UINumber of DEVICE_CAPABILITIES structure.
> -             * This number is typically a user-perceived slot number.
> -             */
> -            if (!SetupDiGetDeviceRegistryProperty(
> -                    parent_dev_info, &parent_dev_info_data, SPDRP_UI_NUMBER,
> -                    &type, (PBYTE)&ui_slot, size, NULL)) {
> -                debug_error("failed to get PCI slot");
> -                ui_slot = -1;
> -                partial_pci = true;
> -            }
> -
> -            /*
> -             * SetupApi gives us the same information as driver with
> -             * IoGetDeviceProperty. According to Microsoft:
> -             *
> -             *   FunctionNumber = (USHORT)((propertyAddress) & 0x0000FFFF)
> -             *   DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF)
> -             *   SPDRP_ADDRESS is propertyAddress, so we do the same.
> -             *
> -             * https://docs.microsoft.com/en-us/windows/desktop/api/setupapi/nf-setupapi-setupdigetdeviceregistrypropertya
> -             */
> -            if (partial_pci) {
> -                pci->domain = -1;
> -                pci->slot = -1;
> -                pci->function = -1;
> -                pci->bus = -1;
> -                continue;
> -            } else {
> -                func = ((int)addr == -1) ? -1 : addr & 0x0000FFFF;
> -                slot = ((int)addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF;
> -                if ((int)ui_slot != slot) {
> -                    g_debug("mismatch with reported slot values: %d vs %d",
> -                            (int)ui_slot, slot);
> -                }
> -                pci->domain = 0;
> -                pci->slot = (int)ui_slot;
> -                pci->function = func;
> -                pci->bus = (int)bus;
> -                break;
> -            }
> -        }
>          break;
>      }
>
> --
> 2.25.1
>



^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 5/6] gqa-win: get_pci_info: Add g_autofree for few variables
  2021-12-14 12:41 ` [PATCH v2 5/6] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk
@ 2021-12-14 13:28   ` Marc-André Lureau
  0 siblings, 0 replies; 13+ messages in thread
From: Marc-André Lureau @ 2021-12-14 13:28 UTC (permalink / raw)
  To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel

On Tue, Dec 14, 2021 at 4:41 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  qga/commands-win32.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 8588fa8633..3092566313 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -626,9 +626,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;
>          SP_DEVINFO_DATA parent_dev_info_data;
>          DWORD size = 0;
>
> @@ -654,7 +654,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>              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)) {
> @@ -741,7 +740,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>          parent_dev_info =
>              SetupDiGetClassDevs(&GUID_DEVINTERFACE_STORAGEPORT, parent_dev_id,
>                                  NULL, DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
> -        g_free(parent_dev_id);
>
>          if (parent_dev_info == INVALID_HANDLE_VALUE) {
>              error_setg_win32(errp, GetLastError(),
> --
> 2.25.1
>



^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 6/6] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function
  2021-12-14 12:41 ` [PATCH v2 6/6] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function Kostiantyn Kostiuk
@ 2021-12-14 13:28   ` Marc-André Lureau
  0 siblings, 0 replies; 13+ messages in thread
From: Marc-André Lureau @ 2021-12-14 13:28 UTC (permalink / raw)
  To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel

On Tue, Dec 14, 2021 at 4:41 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> Microsoft suggests this solution in the documentation:
> https://docs.microsoft.com/en-us/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceinterfacedetaila
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  qga/commands-win32.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3092566313..892082504f 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -636,10 +636,10 @@ 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 (!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 =
> @@ -651,6 +651,16 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>                  }
>              }
>
> +            if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
> +                                                 pdev_iface_detail_data,
> +                                                 size, &size,
> +                                                 &dev_info_data)) {
> +                // pdev_iface_detail_data already is allocated
> +                error_setg_win32(errp, GetLastError(),
> +                                    "failed to get device interfaces");
> +                goto end;
> +            }
> +
>              dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
>                                    FILE_SHARE_READ, NULL, OPEN_EXISTING, 0,
>                                    NULL);
> @@ -680,8 +690,8 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>              ULONG dev_id_size = 0;
>
>              size = 0;
> -            while (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
> -                                               parent_dev_id, size, &size)) {
> +            if (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
> +                                            parent_dev_id, size, &size)) {
>                  if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
>                      parent_dev_id = g_malloc(size);
>                  } else {
> @@ -691,6 +701,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
>                  }
>              }
>
> +            if (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
> +                                            parent_dev_id, size, &size)) {
> +                // parent_dev_id already is allocated
> +                error_setg_win32(errp, GetLastError(),
> +                                    "failed to get device instance ID");
> +                goto end;
> +            }
> +
>              /*
>               * CM API used here as opposed to
>               * SetupDiGetDeviceProperty(..., DEVPKEY_Device_Parent, ...)
> --
> 2.25.1
>



^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-12-14 13:51 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-14 12:41 [PATCH v2 0/6] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
2021-12-14 12:41 ` [PATCH v2 1/6] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk
2021-12-14 13:26   ` Marc-André Lureau
2021-12-14 12:41 ` [PATCH v2 2/6] gqa-win: get_pci_info: Use common 'end' label Kostiantyn Kostiuk
2021-12-14 13:26   ` Marc-André Lureau
2021-12-14 12:41 ` [PATCH v2 3/6] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk
2021-12-14 13:26   ` Marc-André Lureau
2021-12-14 12:41 ` [PATCH v2 4/6] gqa-win: get_pci_info: Split logic to separate functions Kostiantyn Kostiuk
2021-12-14 13:27   ` Marc-André Lureau
2021-12-14 12:41 ` [PATCH v2 5/6] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk
2021-12-14 13:28   ` Marc-André Lureau
2021-12-14 12:41 ` [PATCH v2 6/6] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function Kostiantyn Kostiuk
2021-12-14 13:28   ` Marc-André Lureau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.