Hi Philippe,
On Mon, Aug 9, 2021 at 1:46 PM Philippe Mathieu-Daudé <phi...@redhat.com>
wrote:
> Hi Kostiantyn,
>
> On 8/9/21 11:48 AM, Kostiantyn Kostiuk wrote:
> > Signed-off-by: Kostiantyn Kostiuk <konstan...@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)) {
> ---
>
>
