rdma_get_cm_event error behaviour defined?

rdma_get_cm_event error behaviour defined?

[Dr. David Alan Gilbert]

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.

Dave
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: rdma_get_cm_event error behaviour defined?

[Leon Romanovsky]

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.

Thanks

> 
> Dave
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: rdma_get_cm_event error behaviour defined?

[Dr. David Alan Gilbert]

* 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.

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.

Dave

> Thanks
> 
> > 
> > Dave
> > -- 
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: rdma_get_cm_event error behaviour defined?

[Leon Romanovsky]

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

Re: rdma_get_cm_event error behaviour defined?

[Dr. David Alan Gilbert]

* Leon Romanovsky (leon@kernel.org) wrote:
> 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.

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@redhat.com / Manchester, UK