The guest-get-fsinfo command collects also information about PCI
controller where the disk is attached. When this fails for some reasons
it tries to return just the partial information. However in certain
cases the pointer to the structure was not initialized and was set to
NULL. This breaks the serializer and lead to crasehs of the guest agent.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 qga/commands-win32.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 36d76c22c0..995f62c2e4 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -642,15 +642,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
         g_debug("getting pci-controller info");
         if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad,
                             sizeof(SCSI_ADDRESS), &len, NULL)) {
+            Error *local_err = NULL;
             disk->unit = addr.Lun;
             disk->target = addr.TargetId;
             disk->bus = addr.PathId;
-            disk->pci_controller = get_pci_info(name, errp);
+            g_debug("unit=%lld target=%lld bus=%lld",
+                disk->unit, disk->target, disk->bus);
+            disk->pci_controller = get_pci_info(name, &local_err);
+
+            if (local_err) {
+                slog("failed to get PCI controller info: %s",
+                    error_get_pretty(local_err));
+                error_free(local_err);
+            } else if (disk->pci_controller != NULL) {
+                g_debug("pci: domain=%lld bus=%lld slot=%lld function=%lld",
+                    disk->pci_controller->domain,
+                    disk->pci_controller->bus,
+                    disk->pci_controller->slot,
+                    disk->pci_controller->function);
+            }
         }
-        /* We do not set error in this case, because we still have enough
-         * information about volume. */
-    } else {
-         disk->pci_controller = NULL;
+    }
+    /* We do not set error in case pci_controller is NULL, because we still
+     * have enough information about volume. */
+    if (disk->pci_controller == NULL) {
+        g_debug("no PCI controller info");
+        disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress));
     }
 
     list = g_malloc0(sizeof(*list));
-- 
2.18.0

Quoting Tomáš Golembiovský (2018-08-07 05:51:37)
> The guest-get-fsinfo command collects also information about PCI
> controller where the disk is attached. When this fails for some reasons
> it tries to return just the partial information. However in certain
> cases the pointer to the structure was not initialized and was set to
> NULL. This breaks the serializer and lead to crasehs of the guest agent.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-win32.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 36d76c22c0..995f62c2e4 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -642,15 +642,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
>          g_debug("getting pci-controller info");
>          if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad,
>                              sizeof(SCSI_ADDRESS), &len, NULL)) {
> +            Error *local_err = NULL;
>              disk->unit = addr.Lun;
>              disk->target = addr.TargetId;
>              disk->bus = addr.PathId;
> -            disk->pci_controller = get_pci_info(name, errp);
> +            g_debug("unit=%lld target=%lld bus=%lld",
> +                disk->unit, disk->target, disk->bus);
> +            disk->pci_controller = get_pci_info(name, &local_err);
> +
> +            if (local_err) {
> +                slog("failed to get PCI controller info: %s",
> +                    error_get_pretty(local_err));

slog() is more for logging/auditing events that a guest administrator might
be interested in knowing about, like when qga is accessing files, freezing
filesystems, etc. General qga-side error reporting and debug logging should
go through the normal g_debug/g_warning/etc interfaces to be captured in
qga's log file.

We should also moved patch 1 after this so we don't expose a breakage
prior to the fix.

How often are you seeing failures with the pci info? And does the
information for the non-failures look valid to you? I tried to fix the
CONFIG_QGA_NTDDSCSI naming screw-up a while back and some values like
PCI func/bus/etc looked bogus, SPDRP_BUSNUMBER/SPDRP_ADDRESS/SPDRP_BUSNUMBER
didn't seem to be returning what the current code thinks they are. If that's
still the case it would be good to fix that before we final re-enable this
code.

> +                error_free(local_err);
> +            } else if (disk->pci_controller != NULL) {
> +                g_debug("pci: domain=%lld bus=%lld slot=%lld function=%lld",
> +                    disk->pci_controller->domain,
> +                    disk->pci_controller->bus,
> +                    disk->pci_controller->slot,
> +                    disk->pci_controller->function);
> +            }
>          }
> -        /* We do not set error in this case, because we still have enough
> -         * information about volume. */
> -    } else {
> -         disk->pci_controller = NULL;
> +    }
> +    /* We do not set error in case pci_controller is NULL, because we still
> +     * have enough information about volume. */
> +    if (disk->pci_controller == NULL) {
> +        g_debug("no PCI controller info");
> +        disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress));
>      }
> 
>      list = g_malloc0(sizeof(*list));
> -- 
> 2.18.0
> 

On Wed, 05 Sep 2018 18:21:07 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Quoting Tomáš Golembiovský (2018-08-07 05:51:37)
> > The guest-get-fsinfo command collects also information about PCI
> > controller where the disk is attached. When this fails for some reasons
> > it tries to return just the partial information. However in certain
> > cases the pointer to the structure was not initialized and was set to
> > NULL. This breaks the serializer and lead to crasehs of the guest agent.
> > 
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> >  qga/commands-win32.c | 27 ++++++++++++++++++++++-----
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 36d76c22c0..995f62c2e4 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -642,15 +642,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
> >          g_debug("getting pci-controller info");
> >          if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad,
> >                              sizeof(SCSI_ADDRESS), &len, NULL)) {
> > +            Error *local_err = NULL;
> >              disk->unit = addr.Lun;
> >              disk->target = addr.TargetId;
> >              disk->bus = addr.PathId;
> > -            disk->pci_controller = get_pci_info(name, errp);
> > +            g_debug("unit=%lld target=%lld bus=%lld",
> > +                disk->unit, disk->target, disk->bus);
> > +            disk->pci_controller = get_pci_info(name, &local_err);
> > +
> > +            if (local_err) {
> > +                slog("failed to get PCI controller info: %s",
> > +                    error_get_pretty(local_err));  
> 
> slog() is more for logging/auditing events that a guest administrator might
> be interested in knowing about, like when qga is accessing files, freezing
> filesystems, etc. General qga-side error reporting and debug logging should
> go through the normal g_debug/g_warning/etc interfaces to be captured in
> qga's log file.

ok

> 
> We should also moved patch 1 after this so we don't expose a breakage
> prior to the fix.

ok

> 
> How often are you seeing failures with the pci info?

On Windows 10 Enterprise every the time. On Windows 8 the original code
fails terribly much sooner.

> And does the
> information for the non-failures look valid to you?

I'll tell you when I see that. :)


> I tried to fix the
> CONFIG_QGA_NTDDSCSI naming screw-up a while back and some values like
> PCI func/bus/etc looked bogus, SPDRP_BUSNUMBER/SPDRP_ADDRESS/SPDRP_BUSNUMBER
> didn't seem to be returning what the current code thinks they are. If that's
> still the case it would be good to fix that before we final re-enable this
> code.

Does that mean the queries for SPDRP_* properties work for you?
The issue is that it fails every time as the request is for a
volume not a disk.

Unfortunately I don't know how to fix that at the moment. See my comment
in the followup version of the series that I will send shortly.

    Tomas

> 
> > +                error_free(local_err);
> > +            } else if (disk->pci_controller != NULL) {
> > +                g_debug("pci: domain=%lld bus=%lld slot=%lld function=%lld",
> > +                    disk->pci_controller->domain,
> > +                    disk->pci_controller->bus,
> > +                    disk->pci_controller->slot,
> > +                    disk->pci_controller->function);
> > +            }
> >          }
> > -        /* We do not set error in this case, because we still have enough
> > -         * information about volume. */
> > -    } else {
> > -         disk->pci_controller = NULL;
> > +    }
> > +    /* We do not set error in case pci_controller is NULL, because we still
> > +     * have enough information about volume. */
> > +    if (disk->pci_controller == NULL) {
> > +        g_debug("no PCI controller info");
> > +        disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress));
> >      }
> > 
> >      list = g_malloc0(sizeof(*list));
> > -- 
> > 2.18.0
> >   
> 


-- 
Tomáš Golembiovský <tgolembi@redhat.com>