| Message ID | 20230918144206.560120-53-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration/rdma: Error handling fixes | expand |
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 <armbru@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. > } > > /*
"Zhijian Li (Fujitsu)" <lizhijian@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 <armbru@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!
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); 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"); } /*
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 <armbru@redhat.com> --- migration/rdma.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)