Re: [PATCH 52/52] migration/rdma: Fix how we show device details on open

[Markus Armbruster]

"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!