Ocfs2 devel - Nov 2009 - [patch] Fix: 'return -ENOMEM' instead of 'return ENOMEM'

* Andrew Morton <akpm at 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 at 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]);
 	}

On Thu, Nov 12, 2009 at 6:10 PM, Ingo Molnar <mingo at elte.hu>
wrote:
>
> * Andrew Morton <akpm at 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 at elte.hu>
Looks good to me for radeon bits.

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

Dave.

> * Andrew Morton <akpm at 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

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 at 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 at oracle.com
Phone: (650) 506-8127