On Thu, Sep 27, 2018 at 12:06 PM Tomáš Golembiovský <tgole...@redhat.com> wrote:
> Hi Michael, > > thanks for looking into this. My comments are below. > > Adding Sameeh... > > > On Wed, 26 Sep 2018 12:15:48 -0500 > Michael Roth <mdr...@linux.vnet.ibm.com> wrote: > > > Quoting Tomáš Golembiovský (2018-09-07 06:42:09) > > > 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 leads to a crash of the guest > agent. > > > > > > Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com> > > > > For a win10 guest started with: > > > > qemu-system-x86_64 -m 2G -smp 2,cores=2,sockets=1 -drive > file=/home/mdroth/vm/win10_pro_n_snap0.qcow2,if=virtio -drive > file=/home/mdroth/vm/virtio-win-0.1.102.iso,if=ide,media=cdrom -rtc > base=localtime,driftfix=slew -vga std -boot d -name vm4 -netdev > tap,script=/etc/qemu-ifup,vhost=on,id=vnet0 -device > virtio-net-pci,mac=52:54:00:12:34:04,id=vnic0,netdev=vnet0,disable-modern=true > -vnc :4 -device virtio-serial -balloon virtio -mon chardev=hmp0 -chardev > socket,path=/tmp/vm4-hmp0.sock,server,nowait,id=hmp0 -mon > chardev=qmp0,mode=control -chardev > socket,path=/tmp/vm4-qmp0.sock,server,nowait,id=qmp0 -device > virtserialport,chardev=vs0,name=vs0,id=vs_vs0 -chardev > socket,path=/tmp/vm4-vs0.sock,server,nowait,id=vs0 -device > virtserialport,chardev=vs1,name=vs1,id=vs_vs1 -chardev > socket,path=/tmp/vm4-vs1.sock,server,nowait,id=vs1 -device > virtserialport,chardev=qga,name=org.qemu.guest_agent.0,id=vs_qga -chardev > socket,path=/tmp/vm4-qga.sock,server,nowait,id=qga -device > isa-serial,chardev=serial0,id=serial_serial0 -chardev > socket,path=/tmp/vm4-serial0.sock,server,nowait,id=serial0 -L > /home/mdroth/w/build/qemu-2.11.2-build/pc-bios --enable-kvm > > > > this yields the following: > > > > {'execute':'guest-get-fsinfo'} > > {"return": [{"name": > "\\\\?\\Volume{83835b2d-0032-11e6-a84f-806e6f6e6963}\\", "total-bytes": > 160755712, "mountpoint": "D:\\", "disk": [{"bus-type": "ide", "bus": 0, > "unit": 0, "pci-controller": {"bus": 0, "slot": 0, "domain": 0, "function": > 0}, "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": 0, "slot": 0, "domain": 0, "function": 0}, > "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": 0, "slot": 0, "domain": 0, "function": > 0}, "target": 0}], "used-bytes": 25265487872, "type": "NTFS"}, {"name": > "\\\\?\\Volume{2ea839c6-0000-0000-0000-100000000000}\\", "mountpoint": > "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0, > "pci-controller": {"bus": 0, "slot": 0, "domain": 0, "function": 0}, > "target": 0}], "type": "NTFS"}]} > > > > domain/bus/slot/function=0 are valid PCI addresses so initializing to 0 > is > > wrong. Sameeh had a previous series that initializes to -1 that I think > > is more appropriate (it hasn't gone in yet since we opted not to enable > > CONFIG_QGA_NTDDSCSI for 3.0 since the PCI stuff seems be generally > > broken for Windows, also because the 2nd patch needs some fixups: > Can you please elaborate? What kind of fixups? I can see no comments of this in the 2nd patch > > > > https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg07500.html > > I wasn't aware of that. I is trying to "fix" the same issue. > > I've been also thinking about using -1, but I didn't know what is/isn't > correct PCI address. > > > > > With that series (and some fixups I have on top at > > https://github.com/mdroth/qemu/commits/qga-test), we get the following > > output: > > Should I rebase on that and drop my patch? > > > > > {'execute':'guest-get-fsinfo'} > > {"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": 25267560448, "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"}]} > > > > Here we see the non-sensical PCI topology info I mentioned previously. > > There are values like '"function": 3' even though there are no > > multifunction devices present. This will be exposed to users if we > > enable CONFIG_QGA_NTDDSCSI with things as they stand. > > Sadly, I have no idea how to properly fix this code. But patch 5 of the > series IMHO brings it one step closer to proper solution. The original > code tries to fetch the addr/bus/domain/slot for volume handle (which > fails on missing properties) instead of disk handle. > The remaining issue is that when listing the disks the entries are > cryptic names like "\Device\0000001d" instead of "\PhysicalDriveX" or > "\HarddiskX". And I don't know how to map one name to the other. > char *name = g_strdup(&guid[4]); This needs some investigation... > > > Currently we > > just get an empty array for "disk" field of GuestFilesystemInfo > > for w32, which fortunately aligns with the current QAPI schema (it's > > an array since the volume can span multiple disks). > > No. You get array with one item and useless data. Unless I missed > something. > > > > I'm not about the > > SCSI bus/unit/target data either. It's a real mess (maybe things > > work a bit better when an actual SCSI controller is used?) and I'm > > not sure why/how I didn't notice during initial testing. > > I think unit/target should work. At least with all patches from my > series (well notably the last patch). But please, do verify that if you > can. > > > > I think all this needs to be addressed before we enable > > CONFIG_QGA_NTDDSCSI (in terms of what that's used for in the current > > code at least, i.e. enabling everything reported by > > GuestFilesystemInfo.disk). > > > > What is the oVirt use-case? It doesn't seem like you need PCI topology, > > but what about SCSI topology (and if so, does that information seem > > correct to you)? Or is just a list a serials/dev paths by themselves > > all that's needed? Trying to see how we might decouple things from the > > broken PCI topology stuff. May need to deprecate > > GuestFilesystemInfo.disk in favor of something less monolithic if all > > of this isn't fixable in a reasonable-enough timeframe. > > We don't need the PCI info. I've been only trying to fix this to some > sort along the way. What we need is to get disk serial number (patch 4) > and "name" of the disk of some sort (patch 5). For that I use device > node (e.g. /dev/sda) on Linux and UNC for the disk on Windows (e.g. > \\?\PhysicalDrive1). But the code relies on CONFIG_QGA_NTDDSCSI being > enabled. > This is already enabled in downstream version of qga-win, I don't think it should be enabled upstream if it's not necessary. ( Actually I'm not sure why it is disabled in the first place? I supposed because it is not stable) > > Tomas > > > > > > > > --- > > > 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 98d9735389..9c959122d9 100644 > > > --- a/qga/commands-win32.c > > > +++ b/qga/commands-win32.c > > > @@ -633,15 +633,32 @@ static GuestDiskAddressList > *build_guest_disk_info(char *guid, Error **errp) > > > * > 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)) { > > > + 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) { > > > + g_debug("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 > > > > > > -- > Tomáš Golembiovský <tgole...@redhat.com> >