* [PATCH] migration/rdma: rdma_accept_incoming_migration fix error handling
@ 2020-02-10 19:44 Dr. David Alan Gilbert (git)
2020-02-10 22:33 ` Peter Xu
2020-02-11 11:10 ` Juan Quintela
0 siblings, 2 replies; 6+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2020-02-10 19:44 UTC (permalink / raw)
To: qemu-devel, quintela, peterx
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
rdma_accept_incoming_migration is called from an fd handler and
can't return an Error * anywhere.
Currently it's leaking Error's in errp/local_err - there's
no point putting them in there unless we can report them.
Turn most into fprintf's, and the last into an error_reportf_err
where it's coming up from another function.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
migration/rdma.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/migration/rdma.c b/migration/rdma.c
index 2379b8345b..f67161c98f 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3980,13 +3980,13 @@ static void rdma_accept_incoming_migration(void *opaque)
RDMAContext *rdma = opaque;
int ret;
QEMUFile *f;
- Error *local_err = NULL, **errp = &local_err;
+ Error *local_err = NULL;
trace_qemu_rdma_accept_incoming_migration();
ret = qemu_rdma_accept(rdma);
if (ret) {
- ERROR(errp, "RDMA Migration initialization failed!");
+ fprintf(stderr, "RDMA ERROR: Migration initialization failed");
return;
}
@@ -3998,13 +3998,16 @@ static void rdma_accept_incoming_migration(void *opaque)
f = qemu_fopen_rdma(rdma, "rb");
if (f == NULL) {
- ERROR(errp, "could not qemu_fopen_rdma!");
+ fprintf(stderr, "RDMA ERROR: could not qemu_fopen_rdma");
qemu_rdma_cleanup(rdma);
return;
}
rdma->migration_started_on_destination = 1;
- migration_fd_process_incoming(f, errp);
+ migration_fd_process_incoming(f, &local_err);
+ if (local_err) {
+ error_reportf_err(local_err, "RDMA ERROR:");
+ }
}
void rdma_start_incoming_migration(const char *host_port, Error **errp)
--
2.24.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] migration/rdma: rdma_accept_incoming_migration fix error handling
2020-02-10 19:44 [PATCH] migration/rdma: rdma_accept_incoming_migration fix error handling Dr. David Alan Gilbert (git)
@ 2020-02-10 22:33 ` Peter Xu
2020-02-11 9:22 ` Dr. David Alan Gilbert
2020-02-11 11:10 ` Juan Quintela
1 sibling, 1 reply; 6+ messages in thread
From: Peter Xu @ 2020-02-10 22:33 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: qemu-devel, quintela
On Mon, Feb 10, 2020 at 07:44:59PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> rdma_accept_incoming_migration is called from an fd handler and
> can't return an Error * anywhere.
> Currently it's leaking Error's in errp/local_err - there's
> no point putting them in there unless we can report them.
>
> Turn most into fprintf's, and the last into an error_reportf_err
> where it's coming up from another function.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/rdma.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 2379b8345b..f67161c98f 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -3980,13 +3980,13 @@ static void rdma_accept_incoming_migration(void *opaque)
> RDMAContext *rdma = opaque;
> int ret;
> QEMUFile *f;
> - Error *local_err = NULL, **errp = &local_err;
> + Error *local_err = NULL;
>
> trace_qemu_rdma_accept_incoming_migration();
> ret = qemu_rdma_accept(rdma);
>
> if (ret) {
> - ERROR(errp, "RDMA Migration initialization failed!");
> + fprintf(stderr, "RDMA ERROR: Migration initialization failed");
Is there any reason to explictly use stderr instead of the
error_reportf_err() below (then we simply jump to that for error
paths)? The only difference of error_reportf_err() and stderr should
be when there's one HMP, while shall we always suggest to use
error_reportf_err() rather than stderr?
Thanks,
> return;
> }
>
> @@ -3998,13 +3998,16 @@ static void rdma_accept_incoming_migration(void *opaque)
>
> f = qemu_fopen_rdma(rdma, "rb");
> if (f == NULL) {
> - ERROR(errp, "could not qemu_fopen_rdma!");
> + fprintf(stderr, "RDMA ERROR: could not qemu_fopen_rdma");
> qemu_rdma_cleanup(rdma);
> return;
> }
>
> rdma->migration_started_on_destination = 1;
> - migration_fd_process_incoming(f, errp);
> + migration_fd_process_incoming(f, &local_err);
> + if (local_err) {
> + error_reportf_err(local_err, "RDMA ERROR:");
> + }
> }
>
> void rdma_start_incoming_migration(const char *host_port, Error **errp)
> --
> 2.24.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] migration/rdma: rdma_accept_incoming_migration fix error handling
2020-02-10 22:33 ` Peter Xu
@ 2020-02-11 9:22 ` Dr. David Alan Gilbert
2020-02-11 13:49 ` Peter Xu
0 siblings, 1 reply; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2020-02-11 9:22 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, quintela
* Peter Xu (peterx@redhat.com) wrote:
> On Mon, Feb 10, 2020 at 07:44:59PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > rdma_accept_incoming_migration is called from an fd handler and
> > can't return an Error * anywhere.
> > Currently it's leaking Error's in errp/local_err - there's
> > no point putting them in there unless we can report them.
> >
> > Turn most into fprintf's, and the last into an error_reportf_err
> > where it's coming up from another function.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> > migration/rdma.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index 2379b8345b..f67161c98f 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -3980,13 +3980,13 @@ static void rdma_accept_incoming_migration(void *opaque)
> > RDMAContext *rdma = opaque;
> > int ret;
> > QEMUFile *f;
> > - Error *local_err = NULL, **errp = &local_err;
> > + Error *local_err = NULL;
> >
> > trace_qemu_rdma_accept_incoming_migration();
> > ret = qemu_rdma_accept(rdma);
> >
> > if (ret) {
> > - ERROR(errp, "RDMA Migration initialization failed!");
> > + fprintf(stderr, "RDMA ERROR: Migration initialization failed");
>
> Is there any reason to explictly use stderr instead of the
> error_reportf_err() below (then we simply jump to that for error
> paths)? The only difference of error_reportf_err() and stderr should
> be when there's one HMP, while shall we always suggest to use
> error_reportf_err() rather than stderr?
Because the error_reportf_err is taking an Error* (from an error
reported by migration_fd_process_incoming) where as we don't have an
Error* at the earlier points.
Dave
> Thanks,
>
> > return;
> > }
> >
> > @@ -3998,13 +3998,16 @@ static void rdma_accept_incoming_migration(void *opaque)
> >
> > f = qemu_fopen_rdma(rdma, "rb");
> > if (f == NULL) {
> > - ERROR(errp, "could not qemu_fopen_rdma!");
> > + fprintf(stderr, "RDMA ERROR: could not qemu_fopen_rdma");
> > qemu_rdma_cleanup(rdma);
> > return;
> > }
> >
> > rdma->migration_started_on_destination = 1;
> > - migration_fd_process_incoming(f, errp);
> > + migration_fd_process_incoming(f, &local_err);
> > + if (local_err) {
> > + error_reportf_err(local_err, "RDMA ERROR:");
> > + }
> > }
> >
> > void rdma_start_incoming_migration(const char *host_port, Error **errp)
> > --
> > 2.24.1
> >
>
> --
> Peter Xu
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] migration/rdma: rdma_accept_incoming_migration fix error handling
2020-02-10 19:44 [PATCH] migration/rdma: rdma_accept_incoming_migration fix error handling Dr. David Alan Gilbert (git)
2020-02-10 22:33 ` Peter Xu
@ 2020-02-11 11:10 ` Juan Quintela
2020-02-11 11:15 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 6+ messages in thread
From: Juan Quintela @ 2020-02-11 11:10 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: qemu-devel, peterx
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> rdma_accept_incoming_migration is called from an fd handler and
> can't return an Error * anywhere.
> Currently it's leaking Error's in errp/local_err - there's
> no point putting them in there unless we can report them.
>
> Turn most into fprintf's, and the last into an error_reportf_err
> where it's coming up from another function.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/rdma.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 2379b8345b..f67161c98f 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -3980,13 +3980,13 @@ static void rdma_accept_incoming_migration(void *opaque)
> RDMAContext *rdma = opaque;
> int ret;
> QEMUFile *f;
> - Error *local_err = NULL, **errp = &local_err;
> + Error *local_err = NULL;
>
> trace_qemu_rdma_accept_incoming_migration();
> ret = qemu_rdma_accept(rdma);
>
> if (ret) {
> - ERROR(errp, "RDMA Migration initialization failed!");
> + fprintf(stderr, "RDMA ERROR: Migration initialization failed");
> return;
this should end in "\n" right?
Appart from that.
Reviewed-by: Juan Quintela <quintela@redhat.com>
BTW, I can include the \n when I queue the patch, no need to resend
> }
>
> @@ -3998,13 +3998,16 @@ static void rdma_accept_incoming_migration(void *opaque)
>
> f = qemu_fopen_rdma(rdma, "rb");
> if (f == NULL) {
> - ERROR(errp, "could not qemu_fopen_rdma!");
> + fprintf(stderr, "RDMA ERROR: could not qemu_fopen_rdma");
> qemu_rdma_cleanup(rdma);
> return;
> }
>
> rdma->migration_started_on_destination = 1;
> - migration_fd_process_incoming(f, errp);
> + migration_fd_process_incoming(f, &local_err);
> + if (local_err) {
> + error_reportf_err(local_err, "RDMA ERROR:");
> + }
> }
>
> void rdma_start_incoming_migration(const char *host_port, Error **errp)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] migration/rdma: rdma_accept_incoming_migration fix error handling
2020-02-11 11:10 ` Juan Quintela
@ 2020-02-11 11:15 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2020-02-11 11:15 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, peterx
* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > rdma_accept_incoming_migration is called from an fd handler and
> > can't return an Error * anywhere.
> > Currently it's leaking Error's in errp/local_err - there's
> > no point putting them in there unless we can report them.
> >
> > Turn most into fprintf's, and the last into an error_reportf_err
> > where it's coming up from another function.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> > migration/rdma.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index 2379b8345b..f67161c98f 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -3980,13 +3980,13 @@ static void rdma_accept_incoming_migration(void *opaque)
> > RDMAContext *rdma = opaque;
> > int ret;
> > QEMUFile *f;
> > - Error *local_err = NULL, **errp = &local_err;
> > + Error *local_err = NULL;
> >
> > trace_qemu_rdma_accept_incoming_migration();
> > ret = qemu_rdma_accept(rdma);
> >
> > if (ret) {
> > - ERROR(errp, "RDMA Migration initialization failed!");
> > + fprintf(stderr, "RDMA ERROR: Migration initialization failed");
> > return;
>
> this should end in "\n" right?
Oops thanks; and in the one below.
> Appart from that.
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> BTW, I can include the \n when I queue the patch, no need to resend
Thanks
Dave
>
> > }
> >
> > @@ -3998,13 +3998,16 @@ static void rdma_accept_incoming_migration(void *opaque)
> >
> > f = qemu_fopen_rdma(rdma, "rb");
> > if (f == NULL) {
> > - ERROR(errp, "could not qemu_fopen_rdma!");
> > + fprintf(stderr, "RDMA ERROR: could not qemu_fopen_rdma");
> > qemu_rdma_cleanup(rdma);
> > return;
> > }
> >
> > rdma->migration_started_on_destination = 1;
> > - migration_fd_process_incoming(f, errp);
> > + migration_fd_process_incoming(f, &local_err);
> > + if (local_err) {
> > + error_reportf_err(local_err, "RDMA ERROR:");
> > + }
> > }
> >
> > void rdma_start_incoming_migration(const char *host_port, Error **errp)
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] migration/rdma: rdma_accept_incoming_migration fix error handling
2020-02-11 9:22 ` Dr. David Alan Gilbert
@ 2020-02-11 13:49 ` Peter Xu
0 siblings, 0 replies; 6+ messages in thread
From: Peter Xu @ 2020-02-11 13:49 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: qemu-devel, quintela
On Tue, Feb 11, 2020 at 09:22:00AM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Mon, Feb 10, 2020 at 07:44:59PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > >
> > > rdma_accept_incoming_migration is called from an fd handler and
> > > can't return an Error * anywhere.
> > > Currently it's leaking Error's in errp/local_err - there's
> > > no point putting them in there unless we can report them.
> > >
> > > Turn most into fprintf's, and the last into an error_reportf_err
> > > where it's coming up from another function.
> > >
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > > migration/rdma.c | 11 +++++++----
> > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/migration/rdma.c b/migration/rdma.c
> > > index 2379b8345b..f67161c98f 100644
> > > --- a/migration/rdma.c
> > > +++ b/migration/rdma.c
> > > @@ -3980,13 +3980,13 @@ static void rdma_accept_incoming_migration(void *opaque)
> > > RDMAContext *rdma = opaque;
> > > int ret;
> > > QEMUFile *f;
> > > - Error *local_err = NULL, **errp = &local_err;
> > > + Error *local_err = NULL;
> > >
> > > trace_qemu_rdma_accept_incoming_migration();
> > > ret = qemu_rdma_accept(rdma);
> > >
> > > if (ret) {
> > > - ERROR(errp, "RDMA Migration initialization failed!");
> > > + fprintf(stderr, "RDMA ERROR: Migration initialization failed");
> >
> > Is there any reason to explictly use stderr instead of the
> > error_reportf_err() below (then we simply jump to that for error
> > paths)? The only difference of error_reportf_err() and stderr should
> > be when there's one HMP, while shall we always suggest to use
> > error_reportf_err() rather than stderr?
>
> Because the error_reportf_err is taking an Error* (from an error
> reported by migration_fd_process_incoming) where as we don't have an
> Error* at the earlier points.
The ERROR() macro in rdma.c created them? Though it also prints to
stderr so if we also use the same error_reportf_err() then we can
remove that prints to stderr too.
--
Peter Xu
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-11 13:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10 19:44 [PATCH] migration/rdma: rdma_accept_incoming_migration fix error handling Dr. David Alan Gilbert (git)
2020-02-10 22:33 ` Peter Xu
2020-02-11 9:22 ` Dr. David Alan Gilbert
2020-02-11 13:49 ` Peter Xu
2020-02-11 11:10 ` Juan Quintela
2020-02-11 11:15 ` Dr. David Alan Gilbert
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.