Re: [PATCH 4/4] qemu: Use error from Finish instead of "unexpectedly failed"

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]