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

[address@hidden]

On 03/06/2021 17.30, Philippe Mathieu-Daudé wrote:
> 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().
*

Thanks for your explanation.

Tested-by: Li Zhijian <lizhijian@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 <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;
>>>        }