[PATCH] qemu_migration: check for interface type 'hostdev'

[Laine Stump]

On 7/29/21 5:42 AM, Michal Prívozník wrote:
> On 7/28/21 6:17 PM, Kristina Hanicova wrote:
>> When we try to migrate vm, we check if it contains only devices
>> that are able to migrate. If a hostdev device is not able to
>> migrate we raise an error with <hostdev/>, but it can actually be
>> <interface/>, so we need to check if hostdev device was created
>> by us from interface and show the right error message.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1942315
>>
>> Signed-off-by: Kristina Hanicova <khanicov at redhat.com>
>> ---
>>   src/qemu/qemu_migration.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 4d651aeb1a..34eee9c8b6 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -1272,9 +1272,15 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
>>                   }
>>   
>>                   /* all other PCI hostdevs can't be migrated */
>> -                virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>> -                               _("cannot migrate a domain with <hostdev mode='subsystem' type='%s'>"),
>> -                               virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type));
>> +                if (hostdev->parentnet) {
>> +                    virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>> +                                   _("cannot migrate a domain with <interface type='%s'>"),
>> +                                   virDomainNetTypeToString(hostdev->parentnet->type));
> 
> Small nit, I wonder whether we should report actual type here. Looking
> into virDomainNetDefFormat() it looks like for a running VM we do report
> actual type (unless inactive or migratable XML was requested). Thus I
> think the error message should follow that logic. Otherwise we might
> report "cannot migrate a domain with interface type=network" while in
> fact in 'virsh dumpxml' there is just interface type='hostdev' (the
> type='network' is in inactive XML).
> 
> Laine, what's your opinion?


Well.... neither is ideal :-/. If the log message says "interface 
type='network'" then that could mislead the user into thinking that no 
type='network' interfaces would be supported. But if the log message 
says "interface type='hostdev'" then the user will look in their XML, 
not find any type='hostdev', and then be confused about what the message 
is referencing.

Ideally the error message should make it simple to find the exact 
element causing the problem while also being precise in explaining what 
is problematic. Since neither of the choices here satisfy both, we just 
have to pick the 'least bad' option. I do think in this case that the 
least bad option is to display tha actualtype as you suggest (which, 
BTW, in this case will always be "hostdev").