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

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(a)redhat.com&gt; >> --- >>   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").