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

[Michal Prívozník]

On 8/3/21 3:59 PM, Laine Stump wrote:
> 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.

They would find type='hostdev' in the live XML and type='network' in
inactive one.

> 
> 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").
> 

Agreed. Fixed and pushed.

Michal