[Qemu-devel] [PATCH 0/2] prevent qga from crashing

[Qemu-devel] [PATCH 0/2] prevent qga from crashing

[Sameeh Jubran]

From: Sameeh Jubran <sjubran@redhat.com>

The following patch series fixes an issue with fsinfo command of qemu-ga
on Windows which leads to crash of qga when the disk paramter is
enabled.

Sameeh Jubran (2):
  qga-win: prevent crash when executing fsinfo command
  qga-win: fsinfo: pci-info: allow partial info

 qga/commands-win32.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/2] qga-win: prevent crash when executing fsinfo command

[Sameeh Jubran]

From: Sameeh Jubran <sjubran@redhat.com>

The fsinfo command is currently implemented for Windows only and it's disk
parameter can be enabled by adding the define "CONFIG_QGA_NTDDSCSI" to the qga
code. When enabled and executed the qemu-ga crashed with the following message:

------------------------------------------------
File qapi/qapi-visit-core.c, Line 49

Expression: !(v->type & VISITOR_OUTPUT) || *obj)
------------------------------------------------

After some digging, turns out that the GuestPCIAddress is null and the
qapi visitor doesn't like that, so we can always allocate it instead and
initiate all it's members to -1.

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
---
 qga/commands-win32.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 2d48394748..c5f1c884e1 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -485,6 +485,11 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
     char *buffer = NULL;
     GuestPCIAddress *pci = NULL;
     char *name = g_strdup(&guid[4]);
+    pci = g_malloc0(sizeof(*pci));
+    pci->domain = -1;
+    pci->slot = -1;
+    pci->function = -1;
+    pci->bus = -1;
 
     if (!QueryDosDevice(name, dev_name, ARRAY_SIZE(dev_name))) {
         error_setg_win32(errp, GetLastError(), "failed to get dos device name");
@@ -556,7 +561,6 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
 
         func = addr & 0x0000FFFF;
         dev = (addr >> 16) & 0x0000FFFF;
-        pci = g_malloc0(sizeof(*pci));
         pci->domain = dev;
         pci->slot = slot;
         pci->function = func;
-- 
2.13.6

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

[Sameeh Jubran]

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

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

Re: [Qemu-devel] [PATCH 1/2] qga-win: prevent crash when executing fsinfo command

[Philippe Mathieu-Daudé]

On 06/26/2018 12:10 PM, Sameeh Jubran wrote:
> From: Sameeh Jubran <sjubran@redhat.com>
> 
> The fsinfo command is currently implemented for Windows only and it's disk
> parameter can be enabled by adding the define "CONFIG_QGA_NTDDSCSI" to the qga
> code. When enabled and executed the qemu-ga crashed with the following message:
> 
> ------------------------------------------------
> File qapi/qapi-visit-core.c, Line 49
> 
> Expression: !(v->type & VISITOR_OUTPUT) || *obj)
> ------------------------------------------------

Due to these multiple lines starting with '---', this patch confuses
Thunderbird + colorediffs extension in a funny way...

> 
> After some digging, turns out that the GuestPCIAddress is null and the
> qapi visitor doesn't like that, so we can always allocate it instead and
> initiate all it's members to -1.
> 
> Signed-off-by: Sameeh Jubran <sjubran@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  qga/commands-win32.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 2d48394748..c5f1c884e1 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -485,6 +485,11 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
>      char *buffer = NULL;
>      GuestPCIAddress *pci = NULL;
>      char *name = g_strdup(&guid[4]);
> +    pci = g_malloc0(sizeof(*pci));
> +    pci->domain = -1;
> +    pci->slot = -1;
> +    pci->function = -1;
> +    pci->bus = -1;
>  
>      if (!QueryDosDevice(name, dev_name, ARRAY_SIZE(dev_name))) {
>          error_setg_win32(errp, GetLastError(), "failed to get dos device name");
> @@ -556,7 +561,6 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
>  
>          func = addr & 0x0000FFFF;
>          dev = (addr >> 16) & 0x0000FFFF;
> -        pci = g_malloc0(sizeof(*pci));
>          pci->domain = dev;
>          pci->slot = slot;
>          pci->function = func;
> 

Re: [Qemu-devel] [PATCH 1/2] qga-win: prevent crash when executing fsinfo command

[Eric Blake]

On 06/26/2018 10:10 AM, Sameeh Jubran wrote:
> From: Sameeh Jubran <sjubran@redhat.com>
> 
> The fsinfo command is currently implemented for Windows only and it's disk
> parameter can be enabled by adding the define "CONFIG_QGA_NTDDSCSI" to the qga
> code. When enabled and executed the qemu-ga crashed with the following message:
> 
> ------------------------------------------------
> File qapi/qapi-visit-core.c, Line 49
> 
> Expression: !(v->type & VISITOR_OUTPUT) || *obj)
> ------------------------------------------------
> 
> After some digging, turns out that the GuestPCIAddress is null and the
> qapi visitor doesn't like that, so we can always allocate it instead and
> initiate all it's members to -1.

Adding Markus for a qapi back-compat question.

Is faking an invalid address better than making the output optional instead?

> +++ b/qga/commands-win32.c
> @@ -485,6 +485,11 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
>       char *buffer = NULL;
>       GuestPCIAddress *pci = NULL;
>       char *name = g_strdup(&guid[4]);
> +    pci = g_malloc0(sizeof(*pci));
> +    pci->domain = -1;
> +    pci->slot = -1;
> +    pci->function = -1;
> +    pci->bus = -1;

Right now, we have:

##
# @GuestDiskAddress:
#
# @pci-controller: controller's PCI address
# @bus-type: bus type
# @bus: bus id
# @target: target id
# @unit: unit id
#
# Since: 2.2
##
{ 'struct': 'GuestDiskAddress',
   'data': {'pci-controller': 'GuestPCIAddress',
            'bus-type': 'GuestDiskBusType',
            'bus': 'int', 'target': 'int', 'unit': 'int'} }

and the problem you ran into is that under certain conditions, we have 
no idea what to populate in GuestPCIAddress.  Your patch populates 
garbage instead; but I'm wondering if it would be better to instead mark 
pci-controller as optional, where code that CAN populate it would set 
has_pci_controller=true, and the code that crashed will now work by 
leaving the struct NULL (and has_pci_controller=false).  But that 
removes output that the client may expect to be present, hence, this is 
a back-compat question of the best way to approach this.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/2] qga-win: prevent crash when executing fsinfo command

[Sameeh Jubran]

On Fri, Jun 29, 2018 at 12:44 AM, Eric Blake <eblake@redhat.com> wrote:

> On 06/26/2018 10:10 AM, Sameeh Jubran wrote:
>
>> From: Sameeh Jubran <sjubran@redhat.com>
>>
>> The fsinfo command is currently implemented for Windows only and it's disk
>> parameter can be enabled by adding the define "CONFIG_QGA_NTDDSCSI" to
>> the qga
>> code. When enabled and executed the qemu-ga crashed with the following
>> message:
>>
>> ------------------------------------------------
>> File qapi/qapi-visit-core.c, Line 49
>>
>> Expression: !(v->type & VISITOR_OUTPUT) || *obj)
>> ------------------------------------------------
>>
>> After some digging, turns out that the GuestPCIAddress is null and the
>> qapi visitor doesn't like that, so we can always allocate it instead and
>> initiate all it's members to -1.
>>
>
> Adding Markus for a qapi back-compat question.
>
> Is faking an invalid address better than making the output optional
> instead?
>
> +++ b/qga/commands-win32.c
>> @@ -485,6 +485,11 @@ static GuestPCIAddress *get_pci_info(char *guid,
>> Error **errp)
>>       char *buffer = NULL;
>>       GuestPCIAddress *pci = NULL;
>>       char *name = g_strdup(&guid[4]);
>> +    pci = g_malloc0(sizeof(*pci));
>> +    pci->domain = -1;
>> +    pci->slot = -1;
>> +    pci->function = -1;
>> +    pci->bus = -1;
>>
>
> Right now, we have:
>
> ##
> # @GuestDiskAddress:
> #
> # @pci-controller: controller's PCI address
> # @bus-type: bus type
> # @bus: bus id
> # @target: target id
> # @unit: unit id
> #
> # Since: 2.2
> ##
> { 'struct': 'GuestDiskAddress',
>   'data': {'pci-controller': 'GuestPCIAddress',
>            'bus-type': 'GuestDiskBusType',
>            'bus': 'int', 'target': 'int', 'unit': 'int'} }
>
> and the problem you ran into is that under certain conditions, we have no
> idea what to populate in GuestPCIAddress.  Your patch populates garbage
> instead; but I'm wondering if it would be better to instead mark
> pci-controller as optional, where code that CAN populate it would set
> has_pci_controller=true, and the code that crashed will now work by leaving
> the struct NULL (and has_pci_controller=false).  But that removes output
> that the client may expect to be present, hence, this is a back-compat
> question of the best way to approach this.

Since all of the fields are ints, I believe that "-1" is very informative
even though I don't like this approach but it does preserve backward
compatibility as you've already clarified.

I don't want to assume anything but this bug has been laying around for
quite a while and no one complained, moreover,  the disk option is not
enabled by default in upstream code so I don't think that backward
compatibility is actually disturbed but then again this is just an
assumption.

One more thing to consider is the second patch of this series which
actually was the root cause of why we didn't allocate the " GuestDiskAddress"
struct which is some registry keys being absent for the specific device
which would leave us with two options:
1. Don not return anything for all of the fields (leave it null)
2. Return partial information
I think that the second option is preferable for obvious reasons.

I am not that familiar with schema files, but it is possible to make int
fields optional too?

I can live with either approaches, maintainers, it's your call :)

Thanks for the review!

>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

Re: [Qemu-devel] [PATCH 1/2] qga-win: prevent crash when executing fsinfo command

[Michael Roth]

Quoting Sameeh Jubran (2018-06-28 18:33:58)
> 
> 
> On Fri, Jun 29, 2018 at 12:44 AM, Eric Blake <eblake@redhat.com> wrote:
> 
>     On 06/26/2018 10:10 AM, Sameeh Jubran wrote:
> 
>         From: Sameeh Jubran <sjubran@redhat.com>
> 
>         The fsinfo command is currently implemented for Windows only and it's
>         disk
>         parameter can be enabled by adding the define "CONFIG_QGA_NTDDSCSI" to
>         the qga
>         code. When enabled and executed the qemu-ga crashed with the following
>         message:
> 
>         ------------------------------------------------
>         File qapi/qapi-visit-core.c, Line 49
> 
>         Expression: !(v->type & VISITOR_OUTPUT) || *obj)
>         ------------------------------------------------
> 
>         After some digging, turns out that the GuestPCIAddress is null and the
>         qapi visitor doesn't like that, so we can always allocate it instead
>         and
>         initiate all it's members to -1.
> 
> 
>     Adding Markus for a qapi back-compat question.
> 
>     Is faking an invalid address better than making the output optional
>     instead?
> 
> 
>         +++ b/qga/commands-win32.c
>         @@ -485,6 +485,11 @@ static GuestPCIAddress *get_pci_info(char *guid,
>         Error **errp)
>               char *buffer = NULL;
>               GuestPCIAddress *pci = NULL;
>               char *name = g_strdup(&guid[4]);
>         +    pci = g_malloc0(sizeof(*pci));
>         +    pci->domain = -1;
>         +    pci->slot = -1;
>         +    pci->function = -1;
>         +    pci->bus = -1;
> 
> 
>     Right now, we have:
> 
>     ##
>     # @GuestDiskAddress:
>     #
>     # @pci-controller: controller's PCI address
>     # @bus-type: bus type
>     # @bus: bus id
>     # @target: target id
>     # @unit: unit id
>     #
>     # Since: 2.2
>     ##
>     { 'struct': 'GuestDiskAddress',
>       'data': {'pci-controller': 'GuestPCIAddress',
>                'bus-type': 'GuestDiskBusType',
>                'bus': 'int', 'target': 'int', 'unit': 'int'} }
> 
>     and the problem you ran into is that under certain conditions, we have no
>     idea what to populate in GuestPCIAddress.  Your patch populates garbage
>     instead; but I'm wondering if it would be better to instead mark
>     pci-controller as optional, where code that CAN populate it would set
>     has_pci_controller=true, and the code that crashed will now work by leaving
>     the struct NULL (and has_pci_controller=false).  But that removes output
>     that the client may expect to be present, hence, this is a back-compat
>     question of the best way to approach this.
> 
> Since all of the fields are ints, I believe that "-1" is very informative even
> though I don't like this approach but it does preserve backward compatibility 
> as you've already clarified.
> 
> I don't want to assume anything but this bug has been laying around for quite a
> while and no one complained, moreover,  the disk option is not enabled by
> default in upstream code so I don't think that backward compatibility is
> actually disturbed but then again this is just an assumption.
> 
> One more thing to consider is the second patch of this series which actually
> was the root cause of why we didn't allocate the " GuestDiskAddress" struct
> which is some registry keys being absent for the specific device which would
> leave us with two options:
> 1. Don not return anything for all of the fields (leave it null)
> 2. Return partial information
> I think that the second option is preferable for obvious reasons.
> 
> I am not that familiar with schema files, but it is possible to make int fields
> optional too?
> 
> I can live with either approaches, maintainers, it's your call :)

IMO both approaches potentially break clients, but making the field
optional in this case would be likely to trigger a segfault for any code
that relies on GuestPCIAddress being populated, whereas -1 would likely
just propagate through the stack as a signed integer as the schema calls
for. Lack of -1/undefined handling further up the stack could still
cause breakage but the severity seems lesser. I'm inclined to go this
route for 2.13. If there are no objections I plan to include this in a
pull for 2.13-rc1.

> 
> Thanks for the review!
> 
>    
>     --
>     Eric Blake, Principal Software Engineer
>     Red Hat, Inc.           +1-919-301-3266
>     Virtualization:  qemu.org | libvirt.org
> 
> 
> 
> 
> --
> Respectfully,
> Sameeh Jubran
> Linkedin
> Software Engineer @ Daynix.

Re: [Qemu-devel] [PATCH 1/2] qga-win: prevent crash when executing fsinfo command

[Sameeh Jubran]

I've found another bug which is related to this one, where pci_controller
might also be null:
in the function "build_guest_disk_info" in "qga/commands-win32.c"

if (bus == BusTypeScsi || bus == BusTypeAta || bus == BusTypeRAID

#if (_WIN32_WINNT >= 0x0600)

/* This bus type is not supported before Windows Server 2003 SP1 */

|| bus == BusTypeSas

#endif

) {

/* We are able to use the same ioctls for different bus types

* according to Microsoft docs

* https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */

if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad,

sizeof(SCSI_ADDRESS), &len, NULL)) {

disk->unit = addr.Lun;

disk->target = addr.TargetId;

disk->bus = addr.PathId;

disk->pci_controller = get_pci_info(name, errp);

}

/* We do not set error in this case, because we still have enough

* information about volume. */

} else {

disk->pci_controller = NULL;

}

This can be easily prevented by always calling "get_pci_info(name, errp);",
is this a separate patch, or should be combined with this one? what do you
think?

On Tue, Jul 10, 2018 at 1:58 AM, Michael Roth <mdroth@linux.vnet.ibm.com>
wrote:

> Quoting Sameeh Jubran (2018-06-28 18:33:58)
> >
> >
> > On Fri, Jun 29, 2018 at 12:44 AM, Eric Blake <eblake@redhat.com> wrote:
> >
> >     On 06/26/2018 10:10 AM, Sameeh Jubran wrote:
> >
> >         From: Sameeh Jubran <sjubran@redhat.com>
> >
> >         The fsinfo command is currently implemented for Windows only and
> it's
> >         disk
> >         parameter can be enabled by adding the define
> "CONFIG_QGA_NTDDSCSI" to
> >         the qga
> >         code. When enabled and executed the qemu-ga crashed with the
> following
> >         message:
> >
> >         ------------------------------------------------
> >         File qapi/qapi-visit-core.c, Line 49
> >
> >         Expression: !(v->type & VISITOR_OUTPUT) || *obj)
> >         ------------------------------------------------
> >
> >         After some digging, turns out that the GuestPCIAddress is null
> and the
> >         qapi visitor doesn't like that, so we can always allocate it
> instead
> >         and
> >         initiate all it's members to -1.
> >
> >
> >     Adding Markus for a qapi back-compat question.
> >
> >     Is faking an invalid address better than making the output optional
> >     instead?
> >
> >
> >         +++ b/qga/commands-win32.c
> >         @@ -485,6 +485,11 @@ static GuestPCIAddress *get_pci_info(char
> *guid,
> >         Error **errp)
> >               char *buffer = NULL;
> >               GuestPCIAddress *pci = NULL;
> >               char *name = g_strdup(&guid[4]);
> >         +    pci = g_malloc0(sizeof(*pci));
> >         +    pci->domain = -1;
> >         +    pci->slot = -1;
> >         +    pci->function = -1;
> >         +    pci->bus = -1;
> >
> >
> >     Right now, we have:
> >
> >     ##
> >     # @GuestDiskAddress:
> >     #
> >     # @pci-controller: controller's PCI address
> >     # @bus-type: bus type
> >     # @bus: bus id
> >     # @target: target id
> >     # @unit: unit id
> >     #
> >     # Since: 2.2
> >     ##
> >     { 'struct': 'GuestDiskAddress',
> >       'data': {'pci-controller': 'GuestPCIAddress',
> >                'bus-type': 'GuestDiskBusType',
> >                'bus': 'int', 'target': 'int', 'unit': 'int'} }
> >
> >     and the problem you ran into is that under certain conditions, we
> have no
> >     idea what to populate in GuestPCIAddress.  Your patch populates
> garbage
> >     instead; but I'm wondering if it would be better to instead mark
> >     pci-controller as optional, where code that CAN populate it would set
> >     has_pci_controller=true, and the code that crashed will now work by
> leaving
> >     the struct NULL (and has_pci_controller=false).  But that removes
> output
> >     that the client may expect to be present, hence, this is a
> back-compat
> >     question of the best way to approach this.
> >
> > Since all of the fields are ints, I believe that "-1" is very
> informative even
> > though I don't like this approach but it does preserve backward
> compatibility
> > as you've already clarified.
> >
> > I don't want to assume anything but this bug has been laying around for
> quite a
> > while and no one complained, moreover,  the disk option is not enabled by
> > default in upstream code so I don't think that backward compatibility is
> > actually disturbed but then again this is just an assumption.
> >
> > One more thing to consider is the second patch of this series which
> actually
> > was the root cause of why we didn't allocate the " GuestDiskAddress"
> struct
> > which is some registry keys being absent for the specific device which
> would
> > leave us with two options:
> > 1. Don not return anything for all of the fields (leave it null)
> > 2. Return partial information
> > I think that the second option is preferable for obvious reasons.
> >
> > I am not that familiar with schema files, but it is possible to make int
> fields
> > optional too?
> >
> > I can live with either approaches, maintainers, it's your call :)
>
> IMO both approaches potentially break clients, but making the field
> optional in this case would be likely to trigger a segfault for any code
> that relies on GuestPCIAddress being populated, whereas -1 would likely
> just propagate through the stack as a signed integer as the schema calls
> for. Lack of -1/undefined handling further up the stack could still
> cause breakage but the severity seems lesser. I'm inclined to go this
> route for 2.13. If there are no objections I plan to include this in a
> pull for 2.13-rc1.
>
> >
> > Thanks for the review!
> >
> >
> >     --
> >     Eric Blake, Principal Software Engineer
> >     Red Hat, Inc.           +1-919-301-3266
> >     Virtualization:  qemu.org | libvirt.org
> >
> >
> >
> >
> > --
> > Respectfully,
> > Sameeh Jubran
> > Linkedin
> > Software Engineer @ Daynix.
>
>


-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

Re: [Qemu-devel] [PATCH 2/2] qga-win: fsinfo: pci-info: allow partial info

[Michael Roth]

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
> 

Re: [Qemu-devel] [PATCH 2/2] qga-win: fsinfo: pci-info: allow partial info

[Sameeh Jubran]

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



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*