Re: rdma_get_cm_event error behaviour defined?

[Leon Romanovsky <leon@kernel.org>]

On Tue, Jun 01, 2021 at 12:32:00PM +0100, Dr. David Alan Gilbert wrote:
> * Leon Romanovsky (leon@kernel.org) wrote:
> > On Mon, May 17, 2021 at 11:06:35AM +0100, Dr. David Alan Gilbert wrote:
> > > Hi,
> > >   Is 'rdma_get_cm_event's behaviour in initialising **event
> > > defined in the error case?
> > >   We don't see anything in the manual page, my reading of the
> > > code is it's not set/changed in the case of failure - but is
> > > that defined?
> > >   It would be good if the manpage could explicitly state it.
> > 
> > AFAIK, the general practice do not rely on any output argument if
> > function returns an error and I'm not sure that the man update is
> > needed.
> 
> The case we had was whether we needed to clean up or not in the error
> case; the original code in qemu was:
> 
>     2496     ret = rdma_get_cm_event(rdma->channel, &cm_event);
>     2497     if (ret) {
>     2498         perror("rdma_get_cm_event after rdma_connect");
>     2499         ERROR(errp, "connecting to destination!");
>     2500         rdma_ack_cm_event(cm_event);
>     2501         goto err_rdma_source_connect;
>     2502     }
> 
> and Li spotted that rdma_ack_cm_event  would seg in the case
> rdma_get_cm_event failed.

man page says that you should rdma_ack_cm_event() on success only.

   14 All events which are allocated by rdma_get_cm_event must be released,
   15 there should be a one-to-one correspondence between successful gets
   16 and acks.  This call frees the event structure and any memory that it
   17 references.

> 
> While I agree on not relying on an output; without a definition you're
> stuck between not knowing if you're leaking an event that should
> have been cleaned up.

You are not supposed to have rdma_ack_cm_event() in your snippet.

Thanks