Re: [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event

[Philippe Mathieu-Daudé]

On 6/3/21 3:34 AM, lizhijian@fujitsu.com wrote:
> 
> 
> On 03/06/2021 01.51, Philippe Mathieu-Daudé wrote:
>> Since 00f2cfbbec6 ("glib: bump min required glib library version to
>> 2.48") we can use g_auto/g_autoptr to have the compiler automatically
>> free an allocated variable when it goes out of scope,
> Glad to know this feature.
> 
> However per its code, a  'ack' does much more than just free the memory.
> not sure g_autoptr have the ability to do the same.

See
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#G-DEFINE-AUTOPTR-CLEANUP-FUNC:CAPS

  Defines the appropriate cleanup function for a pointer type.

  The function will not be called if the variable to be cleaned
  up contains NULL.

  This will typically be the _free() or _unref() function for
  the given type.

This does not change the code to call free(ptr), but to call the
registered cleanup function, which is rdma_ack_cm_event().

> 
> 2212 static void ucma_complete_event(struct cma_id_private *id_priv)
> 2213 {
> 2214 pthread_mutex_lock(&id_priv->mut);
> 2215 id_priv->events_completed++;
> 2216 pthread_cond_signal(&id_priv->cond);
> 2217 pthread_mutex_unlock(&id_priv->mut);
> 2218 }
> 2219
> 2220 static void ucma_complete_mc_event(struct cma_multicast *mc)
> 2221 {
> 2222 pthread_mutex_lock(&mc->id_priv->mut);
> 2223 mc->events_completed++;
> 2224 pthread_cond_signal(&mc->cond);
> 2225 mc->id_priv->events_completed++;
> 2226 pthread_cond_signal(&mc->id_priv->cond);
> 2227 pthread_mutex_unlock(&mc->id_priv->mut);
> 2228 }
> 2229
> 2230 int rdma_ack_cm_event(struct rdma_cm_event *event)
> 2231 {
> 2232     struct cma_event *evt;
> 2233
> 2234     if (!event)
> 2235         return ERR(EINVAL);
> 2236
> 2237     evt = container_of(event, struct cma_event, event);
> 2238
> 2239     if (evt->mc)
> 2240 ucma_complete_mc_event(evt->mc);
> 2241 else
> 2242 ucma_complete_event(evt->id_priv);
> 2243 free(evt);
> 2244     return 0;
> 2245 }
> 
> Thanks
> Zhijian
> 
>> removing this
>> burden on the developers.
>>
>> Per rdma_cm(7) and rdma_ack_cm_event(3) man pages:
>>
>>    "rdma_ack_cm_event() - Free a communication event.
>>
>>     All events which are allocated by rdma_get_cm_event() must be
>>     released, there should be a one-to-one correspondence between
>>     successful gets and acks. This call frees the event structure
>>     and any memory that it references."
>>
>> Since the 'ack' description doesn't explicit the event is also
>> released (free'd), it is safer to use the GLib g_autoptr feature.
>> The G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro expects a single word
>> for the type name, so add a type definition to achieve this.
>> Convert to use g_autoptr and remove the rdma_ack_cm_event() calls.
>>
>> Inspired-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> RFC: build-tested only
>> ---
>>   migration/rdma.c | 27 ++++++++++-----------------
>>   1 file changed, 10 insertions(+), 17 deletions(-)
>>
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index b50ebb9183a..b703bf1b918 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -38,6 +38,9 @@
>>   #include "qom/object.h"
>>   #include <poll.h>
>>   
>> +typedef struct rdma_cm_event rdma_cm_event;
>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(rdma_cm_event, rdma_ack_cm_event)
>> +
>>   /*
>>    * Print and error on both the Monitor and the Log file.
>>    */
>> @@ -939,7 +942,7 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, 
>> Error **errp)
>>       int ret;
>>       struct rdma_addrinfo *res;
>>       char port_str[16];
>> -    struct rdma_cm_event *cm_event;
>> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>>       char ip[40] = "unknown";
>>       struct rdma_addrinfo *e;
>>   
>> @@ -1007,11 +1010,11 @@ route:
>>           ERROR(errp, "result not equal to event_addr_resolved %s",
>>                   rdma_event_str(cm_event->event));
>>           perror("rdma_resolve_addr");
>> -        rdma_ack_cm_event(cm_event);
>>           ret = -EINVAL;
>>           goto err_resolve_get_addr;
>>       }