Add fix to _do_madrpc to set the errno after umad_recv.

PATCH In review Sarat Kakarla 2 participants 4 messages Feb 18, 2013 – Mar 18, 2013 2 messages not in the archive InfiniBand

lore

Share & tools

lore

Sarat Kakarla

author

2013-02-18 22:10 UTC #0 lore raw

PATCH 1/1 Add fix to _do_madrpc to set the errno after umad_recv.

_do_madrpc() (in libibmad-1.3.3/src/rpc.c) sends the mad request through umad_send() and waits for a response from umad_recv() with the timeout passed by caller. Linux error
number sent by peer in 'status' is not copied over to the process/thread local 'errno'.

Signed-off-by Sarat Kakarla <[email protected]>

$diff -Naru rpc.c rpc_fix.c

rpc_fix.c 2013-02-18 13:57:15.000000000 -0800 renamed from rpc.c 2013-02-07 10:28:06.000000000 -0800 +1 -0

@@ -175,6 +175,7 @@
                                         IB_MAD_TRID_F) != trid);
 
                status = umad_status(rcvbuf);
+               errno = status;
                if (!status)
                        return length;  /* done */
                if (status == ENOMEM)

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Sarat Kakarla

author

2013-02-25 19:34 UTC #2 lore raw

In reply to a message not in the archive

FOlks,

Just a gentle reminder, whenever you get a chance please review the following fix.

Thanks
Sarat

Quoted 9 lines · Sarat Kakarla

_do_madrpc() (in libibmad-1.3.3/src/rpc.c) sends the mad request through umad_send() and waits for a response from umad_recv() with the timeout passed by caller. Linux error
number sent by peer in 'status' is not copied over to the process/thread local 'errno'.
Signed-off-by Sarat Kakarla <[email protected]>
$diff -Naru rpc.c rpc_fix.c

rpc_fix.c 2013-02-18 13:57:15.000000000 -0800 line 181

Quoted 7 lines

rpc_fix.c 2013-02-18 13:57:15.000000000 -0800 renamed from rpc.c 2013-02-07 10:28:06.000000000 -0800 +1 -0

@@ -175,6 +175,7 @@
                                          IB_MAD_TRID_F) != trid);
 
                 status = umad_status(rcvbuf);
+               errno = status;
                 if (!status)
                         return length;  /* done */
                 if (status == ENOMEM)

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Ira Weiny

2013-02-26 22:29 UTC #3 lore raw

In reply to a message not in the archive

First, it is not documented that this function will set any errno value.

umad_status is only documented to return ETIMEDOUT which will be returned in the p_error parameter (ib_rpc_t -> rstatus) value when an error occurs. Is this not sufficient for your needs?

Secondly, this patch does not apply cleanly. Please create patches against the current master at:

git://git.openfabrics.org/~iraweiny/libibmad.git

Thirdly, the patch below will set errno on each retry case. I don't think this is necessary.

Thanks,
Ira

Quoted 16 lines · On Mon, 18 Feb 2013 14:10:29 -0800 Sarat Kakarla…

_do_madrpc() (in libibmad-1.3.3/src/rpc.c) sends the mad request through umad_send() and waits for a response from umad_recv() with the timeout passed by caller. Linux error
number sent by peer in 'status' is not copied over to the process/thread local 'errno'.
Signed-off-by Sarat Kakarla <[email protected]>
$diff -Naru rpc.c rpc_fix.c
rpc_fix.c 2013-02-18 13:57:15.000000000 -0800 renamed from rpc.c 2013-02-07 10:28:06.000000000 -0800 +1 -0
@@ -175,6 +175,7 @@
                                          IB_MAD_TRID_F) != trid);
 
                 status = umad_status(rcvbuf);
+               errno = status;
                 if (!status)
                         return length;  /* done */
                 if (status == ENOMEM)

Signature · 9 lines

Ira Weiny
Member of Technical Staff
Lawrence Livermore National Lab
925-423-8008
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sarat Kakarla

author

2013-03-18 19:03 UTC #5 lore raw

In reply to a message not in the archive

Ira,

Thanks for the review.

Sorry for the late reply. My answers are in-lined.

Sarat

Quoted 3 lines · Ira Weiny

First, it is not documented that this function will set any errno value.
umad_status is only documented to return ETIMEDOUT which will be returned in the p_error parameter (ib_rpc_t -> rstatus) value when an error occurs. Is this not sufficient for your needs?

We are not invoking the _do_mad_rpc directly, we are invoking ib_resolve_portid_str_via that does not pass-back any of the ib_rpc_t contents, so we just depend on the return code and errno to determine whether a path can be resolved or not.

Quoted 4 lines

Secondly, this patch does not apply cleanly. Please create patches against the current master at:
git://git.openfabrics.org/~iraweiny/libibmad.git

We are working on setting up the the GIT ..

Quoted 2 lines

Thirdly, the patch below will set errno on each retry case. I don't think this is necessary.

Ok I will change the code to return immediately after hitting the E_TIMEDOUT error.

Quoted 29 lines

Quoted 8 lines
                status = umad_status(rcvbuf);
+ errno = status;
                  if (!status)
                     return length;  /* done */
                  if (errno == E_TIMEDOUT)
		        return length;
                  if (status == ENOMEM)
Thanks,
Ira
Quoted 11 lines · On Mon, 18 Feb 2013 14:10:29 -0800 Sarat Kakarla…
_do_madrpc() (in libibmad-1.3.3/src/rpc.c) sends the mad request through umad_send() and waits for a response from umad_recv() with the timeout passed by caller. Linux error
number sent by peer in 'status' is not copied over to the process/thread local 'errno'.
Signed-off-by Sarat Kakarla <[email protected]>
$diff -Naru rpc.c rpc_fix.c
rpc_fix.c 2013-02-18 13:57:15.000000000 -0800 renamed from rpc.c 2013-02-07 10:28:06.000000000 -0800 +0 -0
@@ -175,6 +175,7 @@
                                           IB_MAD_TRID_F) != trid);
 

Quoted 8 lines

rpc_fix.c 2013-02-18 13:57:15.000000000 -0800 line 181
Quoted 5 lines
rpc_fix.c 2013-02-18 13:57:15.000000000 -0800 renamed from rpc.c 2013-02-07 10:28:06.000000000 -0800 +1 -0
                  status = umad_status(rcvbuf);
+               errno = status;
                  if (!status)
                          return length;  /* done */
                  if (status == ENOMEM)

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html