[PATCH] qemu_driver.c: Fix domfsinfo for non-PCI device information from guest agent

[Daniel P. Berrangé]

On Mon, Jul 20, 2020 at 12:22:33PM +0200, Thomas Huth wrote:
> qemuAgentFSInfoToPublic() currently only sets the devAlias for PCI devices.
> However, the QEMU guest agent could also provide the device name in the
> "dev" field of the response for other devices instead (well, at least after
> fixing another problem in the current QEMU guest agent...). So if creating
> the devAlias from the PCI information failed, let's fall back to the name
> provided by the guest agent. This helps to fix the empty "Target" fields
> that occur when running "virsh domfsinfo" on s390x where CCW devices are
> used for the guest instead of PCI devices.
> 
> Also add a proper debug message here in case we completely failed to set the
> device alias, since this problem here was very hard to debug: The only two
> error messages that I've seen were "Unable to get filesystem information"
> and "Unable to encode message payload" - which only indicates that something
> went wrong in the RPC call. No debug message indicated the real problem, so
> I had to learn the hard way why the RPC call failed (it apparently does not
> like devAlias left to be NULL) and where the real problem comes from.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1755075
> Signed-off-by: Thomas Huth <thuth at redhat.com>
> ---
>  src/qemu/qemu_driver.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d185666ed8..e45c61ee20 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -21935,14 +21935,17 @@ qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent,
>          qemuAgentDiskInfoPtr agentdisk = agent->disks[i];
>          virDomainDiskDefPtr diskDef;
>  
> -        if (!(diskDef = virDomainDiskByAddress(vmdef,
> -                                               &agentdisk->pci_controller,
> -                                               agentdisk->bus,
> -                                               agentdisk->target,
> -                                               agentdisk->unit)))
> -            continue;
> -
> -        ret->devAlias[i] = g_strdup(diskDef->dst);
> +        diskDef = virDomainDiskByAddress(vmdef,
> +                                         &agentdisk->pci_controller,
> +                                         agentdisk->bus,
> +                                         agentdisk->target,
> +                                         agentdisk->unit);

Hmmm, even on x86 I think the original code is broken, or at least fragile.
The libvirt "bus" number is not guaranteed to match the guest kernel PCI
"bus" number. I wonder if this has been validated for complex PCI-Express
topologies.

> +        if (diskDef != NULL)
> +            ret->devAlias[i] = g_strdup(diskDef->dst);
> +        else if (agentdisk->devnode != NULL)
> +            ret->devAlias[i] = g_strdup(agentdisk->devnode);

In the linked BZ the "devnode" field is not provided either. You mention
a related QEMU patch, which I guess fixes that issue in QEMU ? Can you point
to that fix.

> +        else
> +            VIR_DEBUG("Missing devnode name for '%s'.", ret->mountpoint);
>      }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|