"Zhijian Li (Fujitsu)" <lizhij...@fujitsu.com> writes: > 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 ?
Yes. >> 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. Makes sense. >> } >> >> /* Thanks!