diff mbox series

[51/52] migration/rdma: Use error_report() & friends instead of stderr

Message ID 20230918144206.560120-52-armbru@redhat.com
State New
Headers show
Series migration/rdma: Error handling fixes | expand

Commit Message

Markus Armbruster Sept. 18, 2023, 2:42 p.m. UTC 
error_report() obeys -msg, reports the current error location if any,
and reports to the current monitor if any.  Reporting to stderr
directly with fprintf() or perror() is wrong, because it loses all
this.

Fix the offenders.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/rdma.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

Comments

Li Zhijian Sept. 26, 2023, 6:35 a.m. UTC | #1 
On 18/09/2023 22:42, Markus Armbruster wrote:
>           if (local->nb_blocks != nb_dest_blocks) {
> -            fprintf(stderr, "ram blocks mismatch (Number of blocks %d vs %d) "
> -                    "Your QEMU command line parameters are probably "
> -                    "not identical on both the source and destination.",
> -                    local->nb_blocks, nb_dest_blocks);
> +            error_report("ram blocks mismatch (Number of blocks %d vs %d)",
> +                         local->nb_blocks, nb_dest_blocks);
> +            error_printf("Your QEMU command line parameters are probably "
> +                         "not identical on both the source and destination.");


Why is this one handled specifically? It seems like it would be fine with error_report().
Markus Armbruster Sept. 27, 2023, 12:16 p.m. UTC | #2 
"Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> writes:

> On 18/09/2023 22:42, Markus Armbruster wrote:
>>           if (local->nb_blocks != nb_dest_blocks) {
>> -            fprintf(stderr, "ram blocks mismatch (Number of blocks %d vs %d) "
>> -                    "Your QEMU command line parameters are probably "
>> -                    "not identical on both the source and destination.",
>> -                    local->nb_blocks, nb_dest_blocks);
>> +            error_report("ram blocks mismatch (Number of blocks %d vs %d)",
>> +                         local->nb_blocks, nb_dest_blocks);
>> +            error_printf("Your QEMU command line parameters are probably "
>> +                         "not identical on both the source and destination.");
>
>
> Why is this one handled specifically? It seems like it would be fine with error_report().

Error message before the patch:

    ram blocks mismatch (Number of blocks %d vs %d) Your QEMU command line parameters are probably not identical on both the source and destination.

Afterwards:

    <ERROR-MSG-PREFIX>ram blocks mismatch (Number of blocks %d vs %d)
    Your QEMU command line parameters are probably not identical on both the source and destination.

where <ERROR-MSG-PREFIX> shows current time (if enabled), guest name (if
enabled), program name (unless in monitor context), and "location" (if
we have one).

The first line is the error message.  It's consists of our common error
message prefix and a single phrase without trailing punctuation.

The second line is a hint.  Repeating the error message prefix there
is unnecessary.  Not repeating it there makes it more obvious that it's
not an error of its own, merely additional information.

Splitting the old message into error message and hint makes the error
message conform to the "single phrase" convention.

Makes sense?
diff mbox series

Patch

diff --git a/migration/rdma.c b/migration/rdma.c
index be2db7946d..9e9904984e 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -871,12 +871,12 @@  static int qemu_rdma_broken_ipv6_kernel(struct ibv_context *verbs, Error **errp)
 
         if (roce_found) {
             if (ib_found) {
-                fprintf(stderr, "WARN: migrations may fail:"
-                                " IPv6 over RoCE / iWARP in linux"
-                                " is broken. But since you appear to have a"
-                                " mixed RoCE / IB environment, be sure to only"
-                                " migrate over the IB fabric until the kernel "
-                                " fixes the bug.\n");
+                warn_report("WARN: migrations may fail:"
+                            " IPv6 over RoCE / iWARP in linux"
+                            " is broken. But since you appear to have a"
+                            " mixed RoCE / IB environment, be sure to only"
+                            " migrate over the IB fabric until the kernel "
+                            " fixes the bug.");
             } else {
                 error_setg(errp, "RDMA ERROR: "
                            "You only have RoCE / iWARP devices in your systems"
@@ -1407,7 +1407,8 @@  static int qemu_rdma_unregister_waiting(RDMAContext *rdma)
         block->remote_keys[chunk] = 0;
 
         if (ret != 0) {
-            perror("unregistration chunk failed");
+            error_report("unregistration chunk failed: %s",
+                         strerror(errno));
             return -1;
         }
         rdma->total_registrations--;
@@ -3751,7 +3752,8 @@  static int qemu_rdma_registration_handle(QEMUFile *f)
                 block->pmr[reg->key.chunk] = NULL;
 
                 if (ret != 0) {
-                    perror("rdma unregistration chunk failed");
+                    error_report("rdma unregistration chunk failed: %s",
+                                 strerror(errno));
                     goto err;
                 }
 
@@ -3940,10 +3942,10 @@  static int qemu_rdma_registration_stop(QEMUFile *f,
          */
 
         if (local->nb_blocks != nb_dest_blocks) {
-            fprintf(stderr, "ram blocks mismatch (Number of blocks %d vs %d) "
-                    "Your QEMU command line parameters are probably "
-                    "not identical on both the source and destination.",
-                    local->nb_blocks, nb_dest_blocks);
+            error_report("ram blocks mismatch (Number of blocks %d vs %d)",
+                         local->nb_blocks, nb_dest_blocks);
+            error_printf("Your QEMU command line parameters are probably "
+                         "not identical on both the source and destination.");
             rdma->errored = true;
             return -1;
         }
@@ -3956,10 +3958,11 @@  static int qemu_rdma_registration_stop(QEMUFile *f,
 
             /* We require that the blocks are in the same order */
             if (rdma->dest_blocks[i].length != local->block[i].length) {
-                fprintf(stderr, "Block %s/%d has a different length %" PRIu64
-                        "vs %" PRIu64, local->block[i].block_name, i,
-                        local->block[i].length,
-                        rdma->dest_blocks[i].length);
+                error_report("Block %s/%d has a different length %" PRIu64
+                             "vs %" PRIu64,
+                             local->block[i].block_name, i,
+                             local->block[i].length,
+                             rdma->dest_blocks[i].length);
                 rdma->errored = true;
                 return -1;
             }
@@ -4075,7 +4078,7 @@  static void rdma_accept_incoming_migration(void *opaque)
     ret = qemu_rdma_accept(rdma);
 
     if (ret < 0) {
-        fprintf(stderr, "RDMA ERROR: Migration initialization failed\n");
+        error_report("RDMA ERROR: Migration initialization failed");
         return;
     }
 
@@ -4087,7 +4090,7 @@  static void rdma_accept_incoming_migration(void *opaque)
 
     f = rdma_new_input(rdma);
     if (f == NULL) {
-        fprintf(stderr, "RDMA ERROR: could not open RDMA for input\n");
+        error_report("RDMA ERROR: could not open RDMA for input");
         qemu_rdma_cleanup(rdma);
         return;
     }