Re: [Qemu-devel] [PATCH v2 3/4] qga: win32: fix crashes when PCI info cannot be retrived

[Michael Roth]

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ý <address@hidden>
> ---
>  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
>