On 03/06/2021 17.30, Philippe Mathieu-Daudé wrote:
> On 6/3/21 3:34 AM, lizhij...@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().
*
Thanks for your explanation.
Tested-by: Li Zhijian <lizhij...@cn.fujitsu.com>
*
>
>> 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 <lizhij...@cn.fujitsu.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <phi...@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;
>>> }
