Re: [dpdk-dev] [PATCH 1/8] ipc: fix rte_mp_request_sync memleak

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 17-Apr-19 3:38 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL.
> 
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: jia.quo@intel.com
> Cc: gi.z.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
>   lib/librte_eal/common/eal_common_proc.c | 6 +++---
>   lib/librte_eal/common/include/rte_eal.h | 3 ++-
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
> index b46d644b3..abaff5164 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
>   
>   	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
>   
> -	if (check_input(req) == false)
> -		return -1;
> -
>   	reply->nb_sent = 0;
>   	reply->nb_received = 0;
>   	reply->msgs = NULL;
>   
> +	if (check_input(req) == false)
> +		return -1;
> +
>   	if (internal_config.no_shconf) {
>   		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
>   		return 0;
> diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
> index 833433229..575f8119e 100644
> --- a/lib/librte_eal/common/include/rte_eal.h
> +++ b/lib/librte_eal/common/include/rte_eal.h
> @@ -309,7 +309,8 @@ rte_mp_sendmsg(struct rte_mp_msg *msg);
>    * This function sends a request message to the peer process, and will
>    * block until receiving reply message from the peer process.
>    *
> - * @note The caller is responsible to free reply->replies.
> + * @note The caller is responsible to free reply->replies (even if the function
> + *    returned failure).
>    *
>    * @param req
>    *   The req argument contains the customized request message.
> 

These patches should've been submitted as one patchset - otherwise it is 
very difficult to review it. Please resubmit as a proper patchset.

As far as the code itself goes,

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>


-- 
Thanks,
Anatoly