[PATCH 1/4] libibmad: Distinguish timed out from other errors

[PATCH 1/4] libibmad: Distinguish timed out from other errors

[Hal Rosenstock]

Add error field like errno to ib_rpc_t structure
In mad_rpc code, make sure error is cleared and
in _do_madrpc, set ETIMEDOUT.

error field could also be used to distinguish other errors if needed.

Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
diff --git a/include/infiniband/mad.h b/include/infiniband/mad.h
index 5d18ec3..0ccdd36 100644
--- a/include/infiniband/mad.h
+++ b/include/infiniband/mad.h
@@ -233,6 +233,7 @@ typedef struct {
 	unsigned recsz;		/* for sa mads (attribute offset) */
 	int timeout;
 	uint32_t oui;		/* for vendor range 2 mads */
+	int error;		/* errno */
 } ib_rpc_t;
 
 typedef struct portid {
diff --git a/src/rpc.c b/src/rpc.c
index a702046..557a33b 100644
--- a/src/rpc.c
+++ b/src/rpc.c
@@ -127,7 +127,7 @@ int mad_rpc_class_agent(struct ibmad_port *port, int class)
 
 static int
 _do_madrpc(int port_id, void *sndbuf, void *rcvbuf, int agentid, int len,
-	   int timeout, int max_retries)
+	   int timeout, int max_retries, int *p_error)
 {
 	uint32_t trid;		/* only low 32 bits */
 	int retries;
@@ -182,6 +182,8 @@ _do_madrpc(int port_id, void *sndbuf, void *rcvbuf, int agentid, int len,
 			return length;
 	}
 
+	if (p_error)
+		*p_error = ETIMEDOUT;
 	ERRS("timeout after %d retries, %d ms", retries, timeout * retries);
 	return -1;
 }
@@ -214,6 +216,7 @@ void *mad_rpc(const struct ibmad_port *port, ib_rpc_t * rpc,
 	uint8_t sndbuf[1024], rcvbuf[1024], *mad;
 	int redirect = 1;
 
+	rpc->error = 0;
 	while (redirect) {
 		len = 0;
 		memset(sndbuf, 0, umad_size() + IB_MAD_SIZE);
@@ -224,7 +227,7 @@ void *mad_rpc(const struct ibmad_port *port, ib_rpc_t * rpc,
 		if ((len = _do_madrpc(port->port_id, sndbuf, rcvbuf,
 				      port->class_agents[rpc->mgtclass],
 				      len, mad_get_timeout(port, rpc->timeout),
-				      mad_get_retries(port))) < 0) {
+				      mad_get_retries(port), &rpc->error)) < 0) {
 			IBWARN("_do_madrpc failed; dport (%s)",
 			       portid2str(dport));
 			return NULL;
@@ -273,13 +276,14 @@ void *mad_rpc_rmpp(const struct ibmad_port *port, ib_rpc_t * rpc,
 
 	DEBUG("rmpp %p data %p", rmpp, data);
 
+	rpc->error = 0;
 	if ((len = mad_build_pkt(sndbuf, rpc, dport, rmpp, data)) < 0)
 		return NULL;
 
 	if ((len = _do_madrpc(port->port_id, sndbuf, rcvbuf,
 			      port->class_agents[rpc->mgtclass],
 			      len, mad_get_timeout(port, rpc->timeout),
-			      mad_get_retries(port))) < 0) {
+			      mad_get_retries(port), &rpc->error)) < 0) {
 		IBWARN("_do_madrpc failed; dport (%s)", portid2str(dport));
 		return NULL;
 	}
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] libibmad: Distinguish timed out from other errors

[Jason Gunthorpe]

On Sat, Apr 09, 2011 at 11:21:39AM -0400, Hal Rosenstock wrote:
> 
> Add error field like errno to ib_rpc_t structure
> In mad_rpc code, make sure error is cleared and
> in _do_madrpc, set ETIMEDOUT.
> 
> error field could also be used to distinguish other errors if needed.

This looks like it breaks the ABI since callers using functions that
accept ib_rpc_t are expected to allocate the ib_rpc_t struct
themselves.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] libibmad: Distinguish timed out from other errors

[Hal Rosenstock]

On 4/9/2011 1:12 PM, Jason Gunthorpe wrote:
> On Sat, Apr 09, 2011 at 11:21:39AM -0400, Hal Rosenstock wrote:
>>
>> Add error field like errno to ib_rpc_t structure
>> In mad_rpc code, make sure error is cleared and
>> in _do_madrpc, set ETIMEDOUT.
>>
>> error field could also be used to distinguish other errors if needed.
> 
> This looks like it breaks the ABI since callers using functions that
> accept ib_rpc_t are expected to allocate the ib_rpc_t struct
> themselves.

Yes, this changes the ABI. I didn't see a good way not to do this. Do
you have any ideas ? The only possible approach I saw was to steal the
high order bit from mgtclass (class is really only a byte) to indicate
which version of ib_rpc_t is being passed as to whether this field is
present.

-- Hal

> Jason
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] libibmad: Distinguish timed out from other errors

[Jason Gunthorpe]

On Sun, Apr 10, 2011 at 06:33:03AM -0400, Hal Rosenstock wrote:
> On 4/9/2011 1:12 PM, Jason Gunthorpe wrote:
> > On Sat, Apr 09, 2011 at 11:21:39AM -0400, Hal Rosenstock wrote:
> >>
> >> Add error field like errno to ib_rpc_t structure
> >> In mad_rpc code, make sure error is cleared and
> >> in _do_madrpc, set ETIMEDOUT.
> >>
> >> error field could also be used to distinguish other errors if needed.
> > 
> > This looks like it breaks the ABI since callers using functions that
> > accept ib_rpc_t are expected to allocate the ib_rpc_t struct
> > themselves.
> 
> Yes, this changes the ABI. I didn't see a good way not to do this. Do
> you have any ideas ? The only possible approach I saw was to steal the
> high order bit from mgtclass (class is really only a byte) to indicate
> which version of ib_rpc_t is being passed as to whether this field is
> present.

If you are going to change the ABI like this then you have to bump the
SONAME.

Setting a flag would work.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] libibmad: Distinguish timed out from other errors

[Hal Rosenstock]

On 4/10/2011 10:09 AM, Jason Gunthorpe wrote:
> On Sun, Apr 10, 2011 at 06:33:03AM -0400, Hal Rosenstock wrote:
>> On 4/9/2011 1:12 PM, Jason Gunthorpe wrote:
>>> On Sat, Apr 09, 2011 at 11:21:39AM -0400, Hal Rosenstock wrote:
>>>>
>>>> Add error field like errno to ib_rpc_t structure
>>>> In mad_rpc code, make sure error is cleared and
>>>> in _do_madrpc, set ETIMEDOUT.
>>>>
>>>> error field could also be used to distinguish other errors if needed.
>>>
>>> This looks like it breaks the ABI since callers using functions that
>>> accept ib_rpc_t are expected to allocate the ib_rpc_t struct
>>> themselves.
>>
>> Yes, this changes the ABI. I didn't see a good way not to do this. Do
>> you have any ideas ? The only possible approach I saw was to steal the
>> high order bit from mgtclass (class is really only a byte) to indicate
>> which version of ib_rpc_t is being passed as to whether this field is
>> present.
> 
> If you are going to change the ABI like this then you have to bump the
> SONAME.

Typically it's been the libibmad maintainer who's bumped the versions
when appropriate as a release is readied.

> Setting a flag would work.

Not sure what you're referring to. Setting a flag where ?

-- Hal

> Jason


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] libibmad: Distinguish timed out from other errors

[Jason Gunthorpe]

On Sun, Apr 10, 2011 at 12:00:05PM -0400, Hal Rosenstock wrote:
> > If you are going to change the ABI like this then you have to bump the
> > SONAME.
> 
> Typically it's been the libibmad maintainer who's bumped the versions
> when appropriate as a release is readied.

The version should be bumped in the same commit that makes the
incompatible change so that people testing the library don't get
messed up by it.
 
> > Setting a flag would work.
> 
> Not sure what you're referring to. Setting a flag where ?

Your idea to place a `new ABI` flag in mgmClass.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] libibmad: Distinguish timed out from other errors

[Hal Rosenstock]

On 4/10/2011 12:44 PM, Jason Gunthorpe wrote:
> On Sun, Apr 10, 2011 at 12:00:05PM -0400, Hal Rosenstock wrote:
>>> If you are going to change the ABI like this then you have to bump the
>>> SONAME.
>>
>> Typically it's been the libibmad maintainer who's bumped the versions
>> when appropriate as a release is readied.
> 
> The version should be bumped in the same commit that makes the
> incompatible change so that people testing the library don't get
> messed up by it.

That's how it used to be done but with the previous maintainer version
bumps were left until release time.

I'll add this to an updated/reworked patch but I hope we'll avoid an
extra last minute/release version bump.

-- Hal

>>> Setting a flag would work.
>>
>> Not sure what you're referring to. Setting a flag where ?
> 
> Your idea to place a `new ABI` flag in mgmClass.

> Jason
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] libibmad: Distinguish timed out from other errors

[Jason Gunthorpe]

On Mon, Apr 11, 2011 at 07:25:05AM -0400, Hal Rosenstock wrote:

> I'll add this to an updated/reworked patch but I hope we'll avoid an
> extra last minute/release version bump.

IMHO you only bump it once during devel, even if two ABI bumping
changes are done. The point is to not mix it up with the already
installed library.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] libibmad: Distinguish timed out from other errors

[Ira Weiny]

On Mon, 11 Apr 2011 08:58:53 -0700
Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:

> On Mon, Apr 11, 2011 at 07:25:05AM -0400, Hal Rosenstock wrote:
> 
> > I'll add this to an updated/reworked patch but I hope we'll avoid an
> > extra last minute/release version bump.

In the end when an official version comes out it is on me to make sure we don't
rev twice.

> 
> IMHO you only bump it once during devel, even if two ABI bumping
> changes are done. The point is to not mix it up with the already
> installed library.

This makes sense to me, and is how I have done things in the past.  Bump the
version and run with that until the official branch/tag.

Ira

> 
> Jason


-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html