diff mbox

[2/2] qga-win: fsinfo: pci-info: allow partial info

Message ID 20180626151038.24771-3-sameeh@daynix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sameeh Jubran June 26, 2018, 3:10 p.m. UTC
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>
---
 qga/commands-win32.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Michael Roth July 16, 2018, 8:04 p.m. UTC | #1
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
>
Sameeh Jubran July 17, 2018, 7:58 a.m. UTC | #2
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 mbox

Patch

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;
     }