Skip to contentSkip to conversation

Return errors from the kernel consistently

PATCH Superseded Jason Gunthorpe 2 participants 3 messages Oct 20, 2009 2 messages not in the archive InfiniBand

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Jason Gunthorpe

author
#0 lore raw

PATCH 1/1 Return errors from the kernel consistently

This fixes one subtle bug, where a return of 0 from the kernel will result in a success report from the library, and fixes a terrible API in the process. Use errno, or return the code, or both. Not half and half..

It is easier to return errno than to fixup the cases that don't, so lets stick with that.

Codes should have been be positive for alignment with POSIX, but it is much too late for that..

Signed-off-by Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 src/cm.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

librdmacm has the same basic flaw too.

What do you think Sean?

src/cm.c +15 -15
index 7370abe..4807e2d 100644
@@ -261,7 +261,7 @@ int ib_cm_destroy_id(struct ib_cm_id *cm_id)
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
 

@@ -292,7 +292,7 @@ int ib_cm_attr_id(struct ib_cm_id *cm_id, struct ib_cm_attr_param *param)
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
 

@@ -322,7 +322,7 @@ int ib_cm_init_qp_attr(struct ib_cm_id *cm_id,
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	VALGRIND_MAKE_MEM_DEFINED(resp, sizeof *resp);
 

@@ -348,7 +348,7 @@ int ib_cm_listen(struct ib_cm_id *cm_id,
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -407,7 +407,7 @@ int ib_cm_send_req(struct ib_cm_id *cm_id, struct ib_cm_req_param *param)
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -442,7 +442,7 @@ int ib_cm_send_rep(struct ib_cm_id *cm_id, struct ib_cm_rep_param *param)
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -467,7 +467,7 @@ static inline int cm_send_private_data(struct ib_cm_id *cm_id,
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -508,7 +508,7 @@ static int cm_establish(struct ib_cm_id *cm_id)
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -533,7 +533,7 @@ int ib_cm_notify(struct ib_cm_id *cm_id, enum ibv_event_type event)
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -567,7 +567,7 @@ static inline int cm_send_status(struct ib_cm_id *cm_id,
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -617,7 +617,7 @@ int ib_cm_send_mra(struct ib_cm_id *cm_id,
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -652,7 +652,7 @@ int ib_cm_send_lap(struct ib_cm_id *cm_id,
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -692,7 +692,7 @@ int ib_cm_send_sidr_req(struct ib_cm_id *cm_id,
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -726,7 +726,7 @@ int ib_cm_send_sidr_rep(struct ib_cm_id *cm_id,
 
 	result = write(cm_id->device->fd, msg, size);
 	if (result != size)
-		return (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 
 	return 0;
 }

@@ -836,7 +836,7 @@ int ib_cm_get_event(struct ib_cm_device *device, struct ib_cm_event **event)
 
 	result = write(device->fd, msg, size);
 	if (result != size) {
-		result = (result > 0) ? -ENODATA : result;
+		return (result >= 0) ? -ENODATA : -1*errno;
 		goto done;
 	}
Signature · 6 lines
1.6.0.4

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

A message that is not in the archive (sent off-list or not yet received): 20091020055834.GA26350-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

#2 lore raw
Quoted 11 lines

This fixes one subtle bug, where a return of 0 from the kernel will result in a success report from the library, and fixes a terrible API in the process. Use errno, or return the code, or both. Not half and half..

It is easier to return errno than to fixup the cases that don't, so lets stick with that.

Codes should have been be positive for alignment with POSIX, but it is much too late for that..

Quoted 6 lines
Signed-off-by Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

---
src/cm.c | 30 +++++++++++++++---------------

1 files changed, 15 insertions(+), 15 deletions(-)

librdmacm has the same basic flaw too.

Thanks - I'll fixup librdmacm the same way.

Is there a reason that you used -1*errno, rather than just -errno?

- Sean

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

A message that is not in the archive (sent off-list or not yet received): 34B1325549824112A02BFE95F2886CB2-Zpru7NauK7drdx17CPfAsdBPR1lH4CV8@public.gmane.org

author
#4 lore raw
Quoted 15 lines · Sean Hefty
Quoted 15 lines

This fixes one subtle bug, where a return of 0 from the kernel will result in a success report from the library, and fixes a terrible API in the process. Use errno, or return the code, or both. Not half and half..

It is easier to return errno than to fixup the cases that don't, so lets stick with that.

Codes should have been be positive for alignment with POSIX, but it is much too late for that..

Signed-off-by Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

src/cm.c | 30 +++++++++++++++---------------

1 files changed, 15 insertions(+), 15 deletions(-)
Quoted 5 lines
Quoted 1 line

librdmacm has the same basic flaw too.

Thanks - I'll fixup librdmacm the same way.

Is there a reason that you used -1*errno, rather than just -errno?

Nope, just do things like that

Actually, please don't apply. I was thinking about this more deeply and it would be much better to bite the bullet and fix the other cases that don't set errno than to do this. Ie always return -1 and remove the -errno thingy entirely.

The documentation says this is OK, and anyone taking shortcuts in this area are more likely to have checked errno then to check the return result, as the return errors are of the rare sort.

I'll send another attempt..

Jason
--
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