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

Thomas Huth thuth at redhat.com
Mon Jul 20 11:05:38 UTC 2020


On 20/07/2020 12.32, Daniel P. Berrangé wrote:
> 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.

I also thought already that maybe I should revert the logic and try
agentdisk->devnode first and only fall back to virDomainDiskByAddress()
if it is not set? ... but that might result in different behavior in
some cases, so I did not use that approach in the this version of my patch.

>> +        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.

I just sent the related patches to qemu-devel (look for the "Allow
guest-get-fsinfo also for non-PCI devices" patch series ... I've also
put you on CC:).

 Thomas




More information about the libvir-list mailing list