On Wed, Jul 08, 2015 at 15:22:52 +0200, Jiri Denemark wrote:
> When QEMU exits on destination during migration, the source reports
> either success (if the failure happened at the very end) or unhelpful
> "unexpectedly failed" error message. However, the Finish API called on
> the destination may report a real error so let's use it instead of the
> generic one.
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
> src/libvirt-domain.c | 21 +++++++++++++++++++--
> src/qemu/qemu_migration.c | 30 ++++++++++++++++++++++++++++--
> 2 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 909c264..f18fee2 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3175,8 +3175,25 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
> (dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen,
> NULL, uri, destflags, cancelled);
> }
> - if (cancelled && ddomain)
> - VIR_ERROR(_("finish step ignored that migration was cancelled"));
> +
See below for comments:
> + if (cancelled) {
> + if (ddomain)
> + VIR_ERROR(_("finish step ignored that migration was cancelled"));
> +
> + /* If Finish reported a useful error, use it instead of the original
> + * "migration unexpectedly failed" error.
> + */
> + if (orig_err &&
> + orig_err->domain == VIR_FROM_QEMU &&
> + orig_err->code == VIR_ERR_OPERATION_FAILED) {
> + virErrorPtr err = virGetLastError();
> + if (err->domain == VIR_FROM_QEMU &&
> + err->code != VIR_ERR_MIGRATE_FINISH_OK) {
> + virFreeError(orig_err);
> + orig_err = NULL;
> + }
> + }
> + }
>
> /* If ddomain is NULL, then we were unable to start
> * the guest on the target, and must restart on the
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 3548d73..d02a0c6 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -4957,8 +4957,25 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver,
> dconnuri, uri, destflags, cancelled);
> qemuDomainObjExitRemote(vm);
> }
> - if (cancelled && ddomain)
> - VIR_ERROR(_("finish step ignored that migration was cancelled"));
> +
The control flow below is weird. Here are a few facts from the code
above:
- ddomain is set via the domainMigrateFinish3(Params) and not anywhere
else
- domainMigrateFinish3 either reports an error or returns ddomain
thus:
> + if (cancelled) {
> + if (ddomain)
> + VIR_ERROR(_("finish step ignored that migration was cancelled"));
> +
Basically all the code below is an else section to the above if
statement due to the previous fact, so ... the below code makes it kind
of opaque.
> + /* If Finish reported a useful error, use it instead of the original
> + * "migration unexpectedly failed" error.
> + */
> + if (orig_err &&
> + orig_err->domain == VIR_FROM_QEMU &&
> + orig_err->code == VIR_ERR_OPERATION_FAILED) {
The code check isn't robust enough IMO. If you want to keep it, the fact
that this happens in a way like this should be noted in a comment for
doNativeMigrate/doTunnelMigrate that set the errors.
> + virErrorPtr err = virGetLastError();
And additionally there is no error reported that this could possibly
overwrite in case where ddomain is not NULL except for the one reported
in virTypedParamsReplaceString but I doubt that will be a better one.
(If that is the intention a comment would really be helpful.
> + if (err->domain == VIR_FROM_QEMU &&
> + err->code != VIR_ERR_MIGRATE_FINISH_OK) {
> + virFreeError(orig_err);
> + orig_err = NULL;
> + }
> + }
> + }
>
> /* If ddomain is NULL, then we were unable to start
> * the guest on the target, and must restart on the
Otherwise makes sense. I'd like to either have an explanation of the
above control flow or a fixed version.
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list