All of lore.kernel.org
 help / color / mirror / Atom feed
From: Anthony Liguori <anthony@codemonkey.ws>
To: Eric Blake <eblake@redhat.com>, qemu-devel@nongnu.org
Cc: qemu-trivial@nongnu.org, pbonzini@redhat.com, phrdina@redhat.com,
	Stefan Hajnoczi <stefanha@redhat.com>,
	quintela@redhat.com
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH] migration: reflect incoming failure to shell
Date: Tue, 16 Apr 2013 17:03:13 -0500	[thread overview]
Message-ID: <87k3o2dtge.fsf@codemonkey.ws> (raw)
In-Reply-To: <1366149041-626-1-git-send-email-eblake@redhat.com>

Eric Blake <eblake@redhat.com> writes:

> Management apps like libvirt don't know to pay attention to
> stderr unless there is a non-zero exit status.
>
> * migration.c (process_incoming_migration_co): Exit with non-zero
> status on failure.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

It looks like the migration coroutine cleans itself so there's no
obvious way to propagate the error.  A nicer cleanup would be to switch
the fprintf to error_report() too but this is certainly better than what
we have now.

Maybe we need an error_report_fatal()...

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori


> ---
>
> Noticed while reviewing:
> https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03293.html
> and it seems blatant enough to fix now, rather than waiting for
> Pavel's series to stabilize.
>
> Side note: libvirt explicitly forbids all use of exit({0,1}), and
> instead encourages exit(EXIT_{SUCCESS,FAILURE}), precisely because
> it makes it harder to slip in unintentional successful exits.
>
>  migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration.c b/migration.c
> index 3b4b467..3eb0fad 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -99,7 +99,7 @@ static void process_incoming_migration_co(void *opaque)
>      qemu_fclose(f);
>      if (ret < 0) {
>          fprintf(stderr, "load of migration failed\n");
> -        exit(0);
> +        exit(EXIT_FAILURE);
>      }
>      qemu_announce_self();
>      DPRINTF("successfully loaded vm state\n");
> -- 
> 1.8.1.4


WARNING: multiple messages have this Message-ID (diff)
From: Anthony Liguori <anthony@codemonkey.ws>
To: Eric Blake <eblake@redhat.com>, qemu-devel@nongnu.org
Cc: qemu-trivial@nongnu.org, pbonzini@redhat.com, phrdina@redhat.com,
	Stefan Hajnoczi <stefanha@redhat.com>,
	quintela@redhat.com
Subject: Re: [Qemu-devel] [PATCH] migration: reflect incoming failure to shell
Date: Tue, 16 Apr 2013 17:03:13 -0500	[thread overview]
Message-ID: <87k3o2dtge.fsf@codemonkey.ws> (raw)
In-Reply-To: <1366149041-626-1-git-send-email-eblake@redhat.com>

Eric Blake <eblake@redhat.com> writes:

> Management apps like libvirt don't know to pay attention to
> stderr unless there is a non-zero exit status.
>
> * migration.c (process_incoming_migration_co): Exit with non-zero
> status on failure.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

It looks like the migration coroutine cleans itself so there's no
obvious way to propagate the error.  A nicer cleanup would be to switch
the fprintf to error_report() too but this is certainly better than what
we have now.

Maybe we need an error_report_fatal()...

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori


> ---
>
> Noticed while reviewing:
> https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03293.html
> and it seems blatant enough to fix now, rather than waiting for
> Pavel's series to stabilize.
>
> Side note: libvirt explicitly forbids all use of exit({0,1}), and
> instead encourages exit(EXIT_{SUCCESS,FAILURE}), precisely because
> it makes it harder to slip in unintentional successful exits.
>
>  migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration.c b/migration.c
> index 3b4b467..3eb0fad 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -99,7 +99,7 @@ static void process_incoming_migration_co(void *opaque)
>      qemu_fclose(f);
>      if (ret < 0) {
>          fprintf(stderr, "load of migration failed\n");
> -        exit(0);
> +        exit(EXIT_FAILURE);
>      }
>      qemu_announce_self();
>      DPRINTF("successfully loaded vm state\n");
> -- 
> 1.8.1.4

  reply	other threads:[~2013-04-16 22:03 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-16 21:50 [Qemu-trivial] [PATCH] migration: reflect incoming failure to shell Eric Blake
2013-04-16 21:50 ` [Qemu-devel] " Eric Blake
2013-04-16 22:03 ` Anthony Liguori [this message]
2013-04-16 22:03   ` Anthony Liguori
2013-04-22 18:35 ` [Qemu-trivial] " Anthony Liguori
2013-04-22 18:35   ` [Qemu-devel] " Anthony Liguori

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87k3o2dtge.fsf@codemonkey.ws \
    --to=anthony@codemonkey.ws \
    --cc=eblake@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=phrdina@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-trivial@nongnu.org \
    --cc=quintela@redhat.com \
    --cc=stefanha@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.