On Wed, 05 Sep 2018 18:21:07 -0500
Michael Roth <mdr...@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ý <tgole...@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ý <tgole...@redhat.com>
