[patch] Fix: 'return -ENOMEM' instead of 'return ENOMEM'

[patch] Fix: 'return -ENOMEM' instead of 'return ENOMEM'

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > @@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
> >  
> >  	s = kmalloc(sizeof(*s), GFP_KERNEL);
> >  	if (!s)
> > -		return ENOMEM;
> > +		return -ENOMEM;
> >  
> >  	trace_seq_init(s);
> >  
> 
> lol, there we go again.
> 
> Andy, can we have a checkpatch rule please?

Note, that will upset creative uses of error codes i guess, such as 
fs/xfs/.

But yeah, +1 from me too.

Ob'post'mortem - looked for similar patterns in the kernel and there's 
quite a few bugs there:

 include/net/inet_hashtables.h:                  return ENOMEM;         # bug
 drivers/scsi/aic7xxx/aic7xxx_osm.c:             return ENOMEM;         # works but weird
 drivers/scsi/cxgb3i/cxgb3i_offload.c:           return ENOMEM;         # works but weird
 fs/ocfs2/dlm/dlmrecovery.c:		return EAGAIN;                  # bug
 drivers/block/cciss_scsi.c:             return ENXIO;                  # works but weird
 drivers/gpu/drm/radeon/radeon_irq.c:                    return EINVAL; # bug
 drivers/gpu/drm/radeon/radeon_irq.c:                    return EINVAL; # bug
 drivers/isdn/hardware/mISDN/hfcmulti.c:         return EINVAL;         # bug

5 out of 8 places look buggy - i.e. more than 60% - a checkpatch warning 
would avoid real bugs here. (even ignoring the cleanliness effects of 
using proper error propagation)

Cc:-ed affected maintainers. The rightmost column are my observations. 
Below is the patch fixing these.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 drivers/gpu/drm/radeon/radeon_irq.c    |    4 ++--
 drivers/isdn/hardware/mISDN/hfcmulti.c |    2 +-
 fs/ocfs2/dlm/dlmrecovery.c             |    2 +-
 include/net/inet_hashtables.h          |    2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_irq.c b/drivers/gpu/drm/radeon/radeon_irq.c
index b79ecc4..fbbc0a1 100644
--- a/drivers/gpu/drm/radeon/radeon_irq.c
+++ b/drivers/gpu/drm/radeon/radeon_irq.c
@@ -76,7 +76,7 @@ int radeon_enable_vblank(struct drm_device *dev, int crtc)
 		default:
 			DRM_ERROR("tried to enable vblank on non-existent crtc %d\n",
 				  crtc);
-			return EINVAL;
+			return -EINVAL;
 		}
 	} else {
 		switch (crtc) {
@@ -89,7 +89,7 @@ int radeon_enable_vblank(struct drm_device *dev, int crtc)
 		default:
 			DRM_ERROR("tried to enable vblank on non-existent crtc %d\n",
 				  crtc);
-			return EINVAL;
+			return -EINVAL;
 		}
 	}
 
diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c b/drivers/isdn/hardware/mISDN/hfcmulti.c
index faed794..cfb45c9 100644
--- a/drivers/isdn/hardware/mISDN/hfcmulti.c
+++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
@@ -2846,7 +2846,7 @@ mode_hfcmulti(struct hfc_multi *hc, int ch, int protocol, int slot_tx,
 	int conf;
 
 	if (ch < 0 || ch > 31)
-		return EINVAL;
+		return -EINVAL;
 	oslot_tx = hc->chan[ch].slot_tx;
 	oslot_rx = hc->chan[ch].slot_rx;
 	conf = hc->chan[ch].conf;
diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
index d9fa3d2..0a8a6a4 100644
--- a/fs/ocfs2/dlm/dlmrecovery.c
+++ b/fs/ocfs2/dlm/dlmrecovery.c
@@ -2639,7 +2639,7 @@ int dlm_begin_reco_handler(struct o2net_msg *msg, u32 len, void *data,
 		     dlm->name, br->node_idx, br->dead_node,
 		     dlm->reco.dead_node, dlm->reco.new_master);
 		spin_unlock(&dlm->spinlock);
-		return EAGAIN;
+		return -EAGAIN;
 	}
 	spin_unlock(&dlm->spinlock);
 
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index d522dcf..5e31447 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -193,7 +193,7 @@ static inline int inet_ehash_locks_alloc(struct inet_hashinfo *hashinfo)
 		hashinfo->ehash_locks =	kmalloc(size * sizeof(spinlock_t),
 						GFP_KERNEL);
 		if (!hashinfo->ehash_locks)
-			return ENOMEM;
+			return -ENOMEM;
 		for (i = 0; i < size; i++)
 			spin_lock_init(&hashinfo->ehash_locks[i]);
 	}

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
--

Re: [patch] Fix: 'return -ENOMEM' instead of 'return ENOMEM'

[Dave Airlie]

On Thu, Nov 12, 2009 at 6:10 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Andrew Morton <akpm@linux-foundation.org> wrote:
>
>> > @@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
>> >
>> >     s = kmalloc(sizeof(*s), GFP_KERNEL);
>> >     if (!s)
>> > -           return ENOMEM;
>> > +           return -ENOMEM;
>> >
>> >     trace_seq_init(s);
>> >
>>
>> lol, there we go again.
>>
>> Andy, can we have a checkpatch rule please?
>
> Note, that will upset creative uses of error codes i guess, such as
> fs/xfs/.
>
> But yeah, +1 from me too.
>
> Ob'post'mortem - looked for similar patterns in the kernel and there's
> quite a few bugs there:
>
>  include/net/inet_hashtables.h:                  return ENOMEM;         # bug
>  drivers/scsi/aic7xxx/aic7xxx_osm.c:             return ENOMEM;         # works but weird
>  drivers/scsi/cxgb3i/cxgb3i_offload.c:           return ENOMEM;         # works but weird
>  fs/ocfs2/dlm/dlmrecovery.c:            return EAGAIN;                  # bug
>  drivers/block/cciss_scsi.c:             return ENXIO;                  # works but weird
>  drivers/gpu/drm/radeon/radeon_irq.c:                    return EINVAL; # bug
>  drivers/gpu/drm/radeon/radeon_irq.c:                    return EINVAL; # bug
>  drivers/isdn/hardware/mISDN/hfcmulti.c:         return EINVAL;         # bug
>
> 5 out of 8 places look buggy - i.e. more than 60% - a checkpatch warning
> would avoid real bugs here. (even ignoring the cleanliness effects of
> using proper error propagation)
>
> Cc:-ed affected maintainers. The rightmost column are my observations.
> Below is the patch fixing these.
>
>        Ingo
>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>

Looks good to me for radeon bits.

Acked-by: Dave Airlie <airlied@redhat.com>

Dave.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
--

Re: [patch] Fix: 'return -ENOMEM' instead of 'return ENOMEM'

[roel kluin]

> * Andrew Morton <akpm@linux-foundation.org> wrote:

>> Andy, can we have a checkpatch rule please?
>
> Note, that will upset creative uses of error codes i guess, such as
> fs/xfs/.
>
> But yeah, +1 from me too.
>
> Ob'post'mortem - looked for similar patterns in the kernel and there's
> quite a few bugs there:
>
>  include/net/inet_hashtables.h:                  return ENOMEM;         # bug
>  drivers/scsi/aic7xxx/aic7xxx_osm.c:             return ENOMEM;         # works but weird
>  drivers/scsi/cxgb3i/cxgb3i_offload.c:           return ENOMEM;         # works but weird
>  fs/ocfs2/dlm/dlmrecovery.c:            return EAGAIN;                  # bug
>  drivers/block/cciss_scsi.c:             return ENXIO;                  # works but weird
>  drivers/gpu/drm/radeon/radeon_irq.c:                    return EINVAL; # bug
>  drivers/gpu/drm/radeon/radeon_irq.c:                    return EINVAL; # bug
>  drivers/isdn/hardware/mISDN/hfcmulti.c:         return EINVAL;         # bug

I noticed some of these as well. I found some more and sent
a few patches last night with a regex like:

git grep -n -E "[^=]=[[:space:]]*E[[:upper:]2]+ *;"

I am not 100% sure this doesn't give many falsies, I am not at home
so cannot test right now. Something like this can show how
common/uncommon these are:

git grep -E "[^=]=[[:space:]]*E[[:upper:]2]+ *;" |
sed -r "s/^([^:]*:).*(return|[^=]=)[[:space:]]*(-?E)[[:upper:]2]+
*;.*$/\1 \3/" |
sort | uniq -c

> 5 out of 8 places look buggy - i.e. more than 60% - a checkpatch warning
> would avoid real bugs here. (even ignoring the cleanliness effects of
> using proper error propagation)

>        Ingo


Thanks for picking this up,

Roel

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
--

Re: [patch] Fix: 'return -ENOMEM' instead of 'return ENOMEM'

[Mike Christie]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

roel kluin wrote:
>> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>>> Andy, can we have a checkpatch rule please?
>> Note, that will upset creative uses of error codes i guess, such as
>> fs/xfs/.
>>
>> But yeah, +1 from me too.
>>
>> Ob'post'mortem - looked for similar patterns in the kernel and there's
>> quite a few bugs there:
>>
>>  include/net/inet_hashtables.h:                  return ENOMEM;         # bug
>>  drivers/scsi/aic7xxx/aic7xxx_osm.c:             return ENOMEM;         # works but weird
>>  drivers/scsi/cxgb3i/cxgb3i_offload.c:           return ENOMEM;         # works but weird

I think cxgb3i is actually in the buggy category. cxgb3i_c3cn_send_pdus 
can propagate the positive EXYZ error value to other functions which 
assume that errors are negative.

Karen, I made the attached patch over James's scsi-rc-fixes tree while 
reviewing the code. Could you test, finish up and send upstream?

[-- Attachment #2: cxgb3i-use-negative-errno.patch --]
[-- Type: text/x-patch, Size: 2523 bytes --]

diff --git a/drivers/scsi/cxgb3i/cxgb3i_offload.c b/drivers/scsi/cxgb3i/cxgb3i_offload.c
index c1d5be4..f6a1fb9 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_offload.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_offload.c
@@ -291,7 +291,7 @@ static void act_open_req_arp_failure(struct t3cdev *dev, struct sk_buff *skb)
 	c3cn_hold(c3cn);
 	spin_lock_bh(&c3cn->lock);
 	if (c3cn->state == C3CN_STATE_CONNECTING)
-		fail_act_open(c3cn, EHOSTUNREACH);
+		fail_act_open(c3cn, -EHOSTUNREACH);
 	spin_unlock_bh(&c3cn->lock);
 	c3cn_put(c3cn);
 	__kfree_skb(skb);
@@ -792,18 +792,18 @@ static int act_open_rpl_status_to_errno(int status)
 {
 	switch (status) {
 	case CPL_ERR_CONN_RESET:
-		return ECONNREFUSED;
+		return -ECONNREFUSED;
 	case CPL_ERR_ARP_MISS:
-		return EHOSTUNREACH;
+		return -EHOSTUNREACH;
 	case CPL_ERR_CONN_TIMEDOUT:
-		return ETIMEDOUT;
+		return -ETIMEDOUT;
 	case CPL_ERR_TCAM_FULL:
-		return ENOMEM;
+		return -ENOMEM;
 	case CPL_ERR_CONN_EXIST:
 		cxgb3i_log_error("ACTIVE_OPEN_RPL: 4-tuple in use\n");
-		return EADDRINUSE;
+		return -EADDRINUSE;
 	default:
-		return EIO;
+		return -EIO;
 	}
 }
 
@@ -817,7 +817,7 @@ static void act_open_retry_timer(unsigned long data)
 	spin_lock_bh(&c3cn->lock);
 	skb = alloc_skb(sizeof(struct cpl_act_open_req), GFP_ATOMIC);
 	if (!skb)
-		fail_act_open(c3cn, ENOMEM);
+		fail_act_open(c3cn, -ENOMEM);
 	else {
 		skb->sk = (struct sock *)c3cn;
 		set_arp_failure_handler(skb, act_open_req_arp_failure);
@@ -966,14 +966,14 @@ static int abort_status_to_errno(struct s3_conn *c3cn, int abort_reason,
 	case CPL_ERR_BAD_SYN: /* fall through */
 	case CPL_ERR_CONN_RESET:
 		return c3cn->state > C3CN_STATE_ESTABLISHED ?
-			EPIPE : ECONNRESET;
+			-EPIPE : -ECONNRESET;
 	case CPL_ERR_XMIT_TIMEDOUT:
 	case CPL_ERR_PERSIST_TIMEDOUT:
 	case CPL_ERR_FINWAIT2_TIMEDOUT:
 	case CPL_ERR_KEEPALIVE_TIMEDOUT:
-		return ETIMEDOUT;
+		return -ETIMEDOUT;
 	default:
-		return EIO;
+		return -EIO;
 	}
 }
 
diff --git a/drivers/scsi/cxgb3i/cxgb3i_pdu.c b/drivers/scsi/cxgb3i/cxgb3i_pdu.c
index 7091050..1fe3b0f 100644
--- a/drivers/scsi/cxgb3i/cxgb3i_pdu.c
+++ b/drivers/scsi/cxgb3i/cxgb3i_pdu.c
@@ -388,8 +388,8 @@ int cxgb3i_conn_xmit_pdu(struct iscsi_task *task)
 	if (err > 0) {
 		int pdulen = err;
 
-	cxgb3i_tx_debug("task 0x%p, skb 0x%p, len %u/%u, rv %d.\n",
-			task, skb, skb->len, skb->data_len, err);
+		cxgb3i_tx_debug("task 0x%p, skb 0x%p, len %u/%u, rv %d.\n",
+				task, skb, skb->len, skb->data_len, err);
 
 		if (task->conn->hdrdgst_en)
 			pdulen += ISCSI_DIGEST_SIZE;

[-- Attachment #3: Type: text/plain, Size: 354 bytes --]

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july

[-- Attachment #4: Type: text/plain, Size: 161 bytes --]

--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [patch] Fix: 'return -ENOMEM' instead of 'return ENOMEM'

[Joel Becker]

On Thu, Nov 12, 2009 at 09:10:43AM +0100, Ingo Molnar wrote:
> 5 out of 8 places look buggy - i.e. more than 60% - a checkpatch warning 
> would avoid real bugs here. (even ignoring the cleanliness effects of 
> using proper error propagation)
> 
> Cc:-ed affected maintainers. The rightmost column are my observations. 
> Below is the patch fixing these.

Acked-by: Joel Becker <joel.becker@oracle.com>


-- 

"Can any of you seriously say the Bill of Rights could get through
 Congress today?  It wouldn't even get out of committee."
	- F. Lee Bailey

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [patch] Fix: 'return -ENOMEM' instead of 'return ENOMEM'

[Joel Becker]

On Thu, Nov 12, 2009 at 11:17:58AM -0800, Joel Becker wrote:
> On Thu, Nov 12, 2009 at 09:10:43AM +0100, Ingo Molnar wrote:
> > 5 out of 8 places look buggy - i.e. more than 60% - a checkpatch warning 
> > would avoid real bugs here. (even ignoring the cleanliness effects of 
> > using proper error propagation)
> > 
> > Cc:-ed affected maintainers. The rightmost column are my observations. 
> > Below is the patch fixing these.
> 
> Acked-by: Joel Becker <joel.becker@oracle.com>

I take that back.  NAK.

Sorry, I read the code wrong.  This function is just a handler.
The caller, dlm_send_begin_reco_message(), expects the positive EAGAIN
as a non-error case.

Joel

-- 

Life's Little Instruction Book #337

	"Reread your favorite book."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [patch] Fix: 'return -ENOMEM' instead of 'return ENOMEM'

[Joel Becker]

On Fri, Nov 13, 2009 at 08:53:06AM +0100, Ingo Molnar wrote:
> > Sorry, I read the code wrong.  This function is just a handler. The 
> > caller, dlm_send_begin_reco_message(), expects the positive EAGAIN as 
> > a non-error case.
> 
> Well, at minimum the error code usage is very confused. The 
> dlm_begin_reco_handler gets registered via o2net_register_handler(). 
> Here's where dlm_begin_reco_handler gets registered, followed by 
> dlm_finalize_reco_handler right afterwards:
<snip> 
> A negative error code right there. The other event handlers are seem to 
> be similar - dlm_begin_reco_handler() is the odd one out.

	The usage of the handlers - 'status' is the return code of the
handler - is pretty straightforward.  Once you're in the mindset of the
o2net code, of course.  This is old stuff, from the earliest days of
ocfs2 development, so we haven't had to spelunk in here for a while ;-)
	Sunil and I both thought while looking at this code that we
could probably return -EAGAIN just as easily, though.  We'll probably
clean it that way at some point.

Joel

-- 

 There are morethings in heaven and earth, Horatio,
 Than are dreamt of in your philosophy.

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127