On 18/09/2023 22:42, Markus Armbruster wrote:
> qemu_rdma_dump_id() dumps RDMA device details to stdout.
>
> rdma_start_outgoing_migration() calls it via qemu_rdma_source_init()
> and qemu_rdma_resolve_host() to show source device details.
> rdma_start_incoming_migration() arranges its call via
> rdma_accept_incoming_migration() and qemu_rdma_accept() to show
> destination device details.
>
> Two issues:
>
> 1. rdma_start_outgoing_migration() can run in HMP context. The
> information should arguably go the monitor, not stdout.
>
> 2. ibv_query_port() failure is reported as error. Its callers remain
> unaware of this failure (qemu_rdma_dump_id() can't fail), so
> reporting this to the user as an error is problematic.
>
> Use qemu_printf() instead of printf() and error_report().
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> migration/rdma.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 9e9904984e..8c84fbab7a 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -30,6 +30,7 @@
> #include "qemu/sockets.h"
> #include "qemu/bitmap.h"
> #include "qemu/coroutine.h"
> +#include "qemu/qemu-print.h"
> #include "exec/memory.h"
> #include <sys/socket.h>
> #include <netdb.h>
> @@ -742,24 +743,25 @@ static void qemu_rdma_dump_id(const char *who, struct
> ibv_context *verbs)
> struct ibv_port_attr port;
>
> if (ibv_query_port(verbs, 1, &port)) {
> - error_report("Failed to query port information");
> + qemu_printf("%s RDMA Device opened, but can't query port
> information",
> + who);
'\n' newline is missing ?
> return;
> }
>
> - printf("%s RDMA Device opened: kernel name %s "
> - "uverbs device name %s, "
> - "infiniband_verbs class device path %s, "
> - "infiniband class device path %s, "
> - "transport: (%d) %s\n",
> + qemu_printf("%s RDMA Device opened: kernel name %s "
> + "uverbs device name %s, "
> + "infiniband_verbs class device path %s, "
> + "infiniband class device path %s, "
> + "transport: (%d) %s\n",
> who,
> verbs->device->name,
> verbs->device->dev_name,
> verbs->device->dev_path,
> verbs->device->ibdev_path,
> port.link_layer,
> - (port.link_layer == IBV_LINK_LAYER_INFINIBAND) ?
> "Infiniband" :
> - ((port.link_layer == IBV_LINK_LAYER_ETHERNET)
> - ? "Ethernet" : "Unknown"));
> + port.link_layer == IBV_LINK_LAYER_INFINIBAND ? "Infiniband"
> + : port.link_layer == IBV_LINK_LAYER_ETHERNET ? "Ethernet"
> + : "Unknown");
Most of the time, these messages are not needed, so i would prefer to put it to
the trace instead.
> }
>
> /*
