* Leon Romanovsky (leon@xxxxxxxxxx) wrote:
> On Tue, Jun 01, 2021 at 12:32:00PM +0100, Dr. David Alan Gilbert wrote:
> > * Leon Romanovsky (leon@xxxxxxxxxx) 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.
Hmm ok; it did fool at least 2 of us; and I ended up going to the code
to check.
> >
> > 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.
Right, that's what we figured out the hard way.
Dave
> Thanks
>
--
Dr. David Alan Gilbert / dgilbert@xxxxxxxxxx / Manchester, UK
