| Message ID | 20200210194459.112085-1-dgilbert@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/rdma: rdma_accept_incoming_migration fix error handling | expand |
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 (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
"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)
* 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
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.
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)