diff mbox series

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

Message ID 20210809094839.52312-3-konstantin@daynix.com (mailing list archive)
State New, archived
Headers show
Series [RFC,1/4] gqa-win: get_pci_info: Clean dev_info if handle is valid | expand

Commit Message

Konstantin Kostiuk Aug. 9, 2021, 9:48 a.m. UTC 
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
 qga/commands-win32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé Aug. 9, 2021, 10:46 a.m. UTC | #1 
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)) {
---
Konstantin Kostiuk Aug. 10, 2021, 10:49 a.m. UTC | #2 
Hi Philippe,

On Mon, Aug 9, 2021 at 1:46 PM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> 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?
>

Yes. This set of patches fix a memory leak. The leak occurs in the case
when a storage device does not have a PCI parent.


>
> > ---
> >  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
>
>
Ok. I will refactor these patches.



> -- >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)) {
> ---
>
>
diff mbox series

Patch

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;