| Message ID | 20180626151038.24771-3-sameeh@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Sameeh Jubran (2018-06-26 10:10:38) > From: Sameeh Jubran <sjubran@redhat.com> > > The call to SetupDiGetDeviceRegistryProperty might fail because the > value doesn't exist in the registry, in this case we shouldn't exit from > the loop but instead continue to look for other available values in the > registry and set this value as unavailable (-1). > > Signed-off-by: Sameeh Jubran <sjubran@redhat.com> The values I'm seeing look off: win7: {'execute':'guest-get-fsinfo','arguments':{}} {"return": [{"name": "\\\\?\\Volume{2823bc2b-b90f-11e7-9ea5-806e6f6e6963}\\", "total-bytes": 316628992, "mountpoint": "E:\\", "disk": [{"bus-type": "ide", "bus": 0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": -1, "function": -1}, "target": 0}], "used-bytes": 316628992, "type": "CDFS"}, {"name": "\\\\?\\Volume{a1ed8064-3f4a-11e1-972d-806e6f6e6963}\\", "total-bytes": 21367877632, "mountpoint": "C:\\", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 2}, "target": 0}], "used-bytes": 19656269824, "type": "NTFS"}, {"name": "\\\\?\\Volume{a1ed8063-3f4a-11e1-972d-806e6f6e6963}\\", "mountpoint": "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 1}, "target": 0}], "type": "NTFS"}]} win10: {'execute':'guest-get-fsinfo','arguments':{}} {"return": [{"name": "\\\\?\\Volume{83835b2d-0032-11e6-a84f-806e6f6e6963}\\", "total-bytes": 160755712, "mountpoint": "D:\\", "disk": [{"bus-type": "ide", "bus": 0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": -1, "function": -1}, "target": 0}], "used-bytes": 160755712, "type": "CDFS"}, {"name": "\\\\?\\Volume{2ea839c6-0000-0000-0000-80620c000000}\\", "mountpoint": "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 3}, "target": 0}], "type": "NTFS"}, {"name": "\\\\?\\Volume{2ea839c6-0000-0000-0000-501f00000000}\\", "total-bytes": 52665839616, "mountpoint": "C:\\", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 2}, "target": 0}], "used-bytes": 29645811712, "type": "NTFS"}, {"name": "\\\\?\\Volume{2ea839c6-0000-0000-0000-100000000000}\\", "mountpoint": "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 1}, "target": 0}], "type": "NTFS"}]} If any one of domain/bus/slot/func fail, we should initialize everything to -1 since we can't partially report a PCI address. The fact that we get partial success makes me think SPDRP_ADDRESS, SPDRP_UI_NUMBER, etc. aren't reporting what we expect they should. The documentation seems a bit nebulous. Also I'm not using multifunction to the non-0 function values seem off. Do you know of anyone manually setting CONFIG_QGA_NTDDSCSI in practice? It's been left disabled in configure (left misnamed as CONFIG_QGA_NTDDDISK due to some problems that popped up when we tried to enable it that I don't quite recall, maybe similar issues to what you're seeing). I'm starting to think it's better to leave this for 3.1 since it's not technically a supported feature yet and may need some reworking outside of the issue you were originally addressing. > --- > qga/commands-win32.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index c5f1c884e1..55e460dee3 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -505,7 +505,8 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) > > dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA); > for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) { > - DWORD addr, bus, slot, func, dev, data, size2; > + DWORD addr, bus, slot, data, size2; > + int func, dev; > while (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, > SPDRP_PHYSICAL_DEVICE_OBJECT_NAME, > &data, (PBYTE)buffer, size, > @@ -535,21 +536,21 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) > */ > if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, > SPDRP_BUSNUMBER, &data, (PBYTE)&bus, size, NULL)) { > - break; > + bus = -1; > } > > /* 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, &data, (PBYTE)&addr, size, NULL)) { > - break; > + addr = -1; > } > > /* 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, &data, (PBYTE)&slot, size, NULL)) { > - break; > + slot = -1; > } > > /* SetupApi gives us the same information as driver with > @@ -559,12 +560,12 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) > * DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF); > * SPDRP_ADDRESS is propertyAddress, so we do the same.*/ > > - func = addr & 0x0000FFFF; > - dev = (addr >> 16) & 0x0000FFFF; > + func = ((int) addr == -1) ? -1 : addr & 0x0000FFFF; > + dev = ((int) addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF; > pci->domain = dev; > - pci->slot = slot; > + pci->slot = (int) slot; > pci->function = func; > - pci->bus = bus; > + pci->bus = (int) bus; > break; > } > > -- > 2.13.6 >
On Mon, Jul 16, 2018 at 11:04 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > Quoting Sameeh Jubran (2018-06-26 10:10:38) > > From: Sameeh Jubran <sjubran@redhat.com> > > > > The call to SetupDiGetDeviceRegistryProperty might fail because the > > value doesn't exist in the registry, in this case we shouldn't exit from > > the loop but instead continue to look for other available values in the > > registry and set this value as unavailable (-1). > > > > Signed-off-by: Sameeh Jubran <sjubran@redhat.com> > > The values I'm seeing look off: > > win7: > > {'execute':'guest-get-fsinfo','arguments':{}} > > {"return": [{"name": > "\\\\?\\Volume{2823bc2b-b90f-11e7-9ea5-806e6f6e6963}\\", "total-bytes": > 316628992, "mountpoint": "E:\\", "disk": [{"bus-type": "ide", "bus": 0, > "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": -1, > "function": -1}, "target": 0}], "used-bytes": 316628992, "type": > "CDFS"}, {"name": > "\\\\?\\Volume{a1ed8064-3f4a-11e1-972d-806e6f6e6963}\\", "total-bytes": > 21367877632, "mountpoint": "C:\\", "disk": [{"bus-type": "scsi", "bus": > 0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0, > "function": 2}, "target": 0}], "used-bytes": 19656269824, "type": > "NTFS"}, {"name": > "\\\\?\\Volume{a1ed8063-3f4a-11e1-972d-806e6f6e6963}\\", "mountpoint": > "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, > "pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 1}, > "target": 0}], "type": "NTFS"}]} > > win10: > > {'execute':'guest-get-fsinfo','arguments':{}} > > {"return": [{"name": > "\\\\?\\Volume{83835b2d-0032-11e6-a84f-806e6f6e6963}\\", "total-bytes": > 160755712, "mountpoint": "D:\\", "disk": [{"bus-type": "ide", "bus": 0, > "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": -1, > "function": -1}, "target": 0}], "used-bytes": 160755712, "type": > "CDFS"}, {"name": > "\\\\?\\Volume{2ea839c6-0000-0000-0000-80620c000000}\\", "mountpoint": > "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, > "pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 3}, > "target": 0}], "type": "NTFS"}, {"name": > "\\\\?\\Volume{2ea839c6-0000-0000-0000-501f00000000}\\", "total-bytes": > 52665839616, "mountpoint": "C:\\", "disk": [{"bus-type": "scsi", "bus": > 0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0, > "function": 2}, "target": 0}], "used-bytes": 29645811712, "type": > "NTFS"}, {"name": > "\\\\?\\Volume{2ea839c6-0000-0000-0000-100000000000}\\", "mountpoint": > "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, > "pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 1}, > "target": 0}], "type": "NTFS"}]} > > If any one of domain/bus/slot/func fail, we should initialize everything > to -1 since we can't partially report a PCI address. The fact that we > get partial success makes me think SPDRP_ADDRESS, SPDRP_UI_NUMBER, etc. > aren't reporting what we expect they should. The documentation seems > a bit nebulous. Also I'm not using multifunction to the non-0 function > values seem off. > > > Do you know of anyone manually setting CONFIG_QGA_NTDDSCSI in practice? > Yes, I am using it actually. > It's been left disabled in configure (left misnamed as CONFIG_QGA_NTDDDISK > due to some problems that popped up when we tried to enable it that I > don't quite recall, maybe similar issues to what you're seeing). I'm > starting to think it's better to leave this for 3.1 since it's not > technically a supported feature yet and may need some reworking outside > of the issue you were originally addressing. > No problem it can wait for 3.1. > > > --- > > qga/commands-win32.c | 17 +++++++++-------- > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > > index c5f1c884e1..55e460dee3 100644 > > --- a/qga/commands-win32.c > > +++ b/qga/commands-win32.c > > @@ -505,7 +505,8 @@ static GuestPCIAddress *get_pci_info(char *guid, > Error **errp) > > > > dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA); > > for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); > i++) { > > - DWORD addr, bus, slot, func, dev, data, size2; > > + DWORD addr, bus, slot, data, size2; > > + int func, dev; > > while (!SetupDiGetDeviceRegistryProperty(dev_info, > &dev_info_data, > > > SPDRP_PHYSICAL_DEVICE_OBJECT_NAME, > > &data, (PBYTE)buffer, size, > > @@ -535,21 +536,21 @@ static GuestPCIAddress *get_pci_info(char *guid, > Error **errp) > > */ > > if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, > > SPDRP_BUSNUMBER, &data, (PBYTE)&bus, size, NULL)) { > > - break; > > + bus = -1; > > } > > > > /* 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, &data, (PBYTE)&addr, size, NULL)) { > > - break; > > + addr = -1; > > } > > > > /* 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, &data, (PBYTE)&slot, size, NULL)) { > > - break; > > + slot = -1; > > } > > > > /* SetupApi gives us the same information as driver with > > @@ -559,12 +560,12 @@ static GuestPCIAddress *get_pci_info(char *guid, > Error **errp) > > * DeviceNumber = (USHORT)(((propertyAddress) >> 16) & > 0x0000FFFF); > > * SPDRP_ADDRESS is propertyAddress, so we do the same.*/ > > > > - func = addr & 0x0000FFFF; > > - dev = (addr >> 16) & 0x0000FFFF; > > + func = ((int) addr == -1) ? -1 : addr & 0x0000FFFF; > > + dev = ((int) addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF; > > pci->domain = dev; > > - pci->slot = slot; > > + pci->slot = (int) slot; > > pci->function = func; > > - pci->bus = bus; > > + pci->bus = (int) bus; > > break; > > } > > > > -- > > 2.13.6 > > >
diff --git a/qga/commands-win32.c b/qga/commands-win32.c index c5f1c884e1..55e460dee3 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -505,7 +505,8 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA); for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) { - DWORD addr, bus, slot, func, dev, data, size2; + DWORD addr, bus, slot, data, size2; + int func, dev; while (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, SPDRP_PHYSICAL_DEVICE_OBJECT_NAME, &data, (PBYTE)buffer, size, @@ -535,21 +536,21 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) */ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, SPDRP_BUSNUMBER, &data, (PBYTE)&bus, size, NULL)) { - break; + bus = -1; } /* 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, &data, (PBYTE)&addr, size, NULL)) { - break; + addr = -1; } /* 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, &data, (PBYTE)&slot, size, NULL)) { - break; + slot = -1; } /* SetupApi gives us the same information as driver with @@ -559,12 +560,12 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) * DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF); * SPDRP_ADDRESS is propertyAddress, so we do the same.*/ - func = addr & 0x0000FFFF; - dev = (addr >> 16) & 0x0000FFFF; + func = ((int) addr == -1) ? -1 : addr & 0x0000FFFF; + dev = ((int) addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF; pci->domain = dev; - pci->slot = slot; + pci->slot = (int) slot; pci->function = func; - pci->bus = bus; + pci->bus = (int) bus; break; }