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)) {
---