[PATCH] ftrace: return error instead of 12 bytes read

[PATCH] ftrace: return error instead of 12 bytes read

[Roel Kluin]

A negative error value is required: now we cannot
distinguish ENOMEM from a valid read of 12 bytes.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b20d3ec..03c7fd5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -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);
 

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Andrew Morton]

On Wed, 11 Nov 2009 22:26:35 +0100
Roel Kluin <roel.kluin@gmail.com> wrote:

> A negative error value is required: now we cannot
> distinguish ENOMEM from a valid read of 12 bytes.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
>  kernel/trace/trace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index b20d3ec..03c7fd5 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -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?

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Steven Rostedt]

On Wed, 2009-11-11 at 13:47 -0800, Andrew Morton wrote:
> On Wed, 11 Nov 2009 22:26:35 +0100
> Roel Kluin <roel.kluin@gmail.com> wrote:
> 
> > A negative error value is required: now we cannot
> > distinguish ENOMEM from a valid read of 12 bytes.
> > 
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> >  kernel/trace/trace.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index b20d3ec..03c7fd5 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -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?

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

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

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

[Ocfs2-devel] [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]);
 	}

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'

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

[Ocfs2-devel] [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.

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'

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

[Ocfs2-devel] [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

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

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

[Mike Christie]

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cxgb3i-use-negative-errno.patch
Type: text/x-patch
Size: 2523 bytes
Desc: not available
Url : http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20091112/09f1764b/attachment.bin 

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

[Alexey Dobriyan]

On 11/12/09, Ingo Molnar <mingo@elte.hu> wrote:
>  drivers/scsi/aic7xxx/aic7xxx_osm.c:             return ENOMEM;         # works but weird

"weirdness" comes from BSD where E codes are negative,
so they do not have this problem at all.

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

[Ocfs2-devel] [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 at 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 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

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

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

[Ingo Molnar]

* Joel Becker <Joel.Becker@oracle.com> wrote:

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

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:

        status = o2net_register_handler(DLM_BEGIN_RECO_MSG, dlm->key,
                                        sizeof(struct dlm_begin_reco),
                                        dlm_begin_reco_handler,
                                        dlm, NULL, &dlm->dlm_domain_handlers);

and right before that dlm_reco_data_done_handler() gets registered:

        status = o2net_register_handler(DLM_RECO_DATA_DONE_MSG, dlm->key,
                                        sizeof(struct dlm_reco_data_done),
                                        dlm_reco_data_done_handler,
                                        dlm, NULL, &dlm->dlm_domain_handlers);

And look what dlm_reco_data_done_handler() starts with:

int dlm_reco_data_done_handler(struct o2net_msg *msg, u32 len, void *data,
                               void **ret_data)
{
        struct dlm_ctxt *dlm = data;
        struct dlm_reco_data_done *done = (struct dlm_reco_data_done *)msg->buf;
        struct dlm_reco_node_data *ndata = NULL;
        int ret = -EINVAL;

        if (!dlm_grab(dlm))
                return -EINVAL;

A negative error code right there. The other event handlers are seem to 
be similar - dlm_begin_reco_handler() is the odd one out.

So while you are right and my patch is wrong and DLM_BEGIN_RECO_MSG 
processing works right now - at minimum this is a very dangerous/fragile 
pattern of error code usage. For exampe if anyone uses 
dlm_begin_reco_handler() to implement a new state machine handler in the 
future, but uses one of the other event handlers to actually use it, a 
hard to find bug is introduced.

	Ingo

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

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

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

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Andy Whitcroft]

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

Thats a tricky one.  Not only do we not really have a sensible way to
know if ENOMEM is an errno, we also find a bunch of places that we
appear to use positive errno's as return values where we would falsly
complain about.  Its particularly common in scsi and filesystems.
Admittedly the vast majority are return -EXXX form, so we could add
this as a non-default check perhaps.

Thoughts?

-apw

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Ingo Molnar]

* Andy Whitcroft <apw@canonical.com> wrote:

> >> ? ? ? 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?
> 
> Thats a tricky one.  Not only do we not really have a sensible way to 
> know if ENOMEM is an errno, we also find a bunch of places that we 
> appear to use positive errno's as return values where we would falsly 
> complain about.  Its particularly common in scsi and filesystems. 
> Admittedly the vast majority are return -EXXX form, so we could add 
> this as a non-default check perhaps.
> 
> Thoughts?

Even in filesystems, ~80% of the cases use proper negative values:

 $ git grep 'return -E' fs/ | wc -l
 4540
 $ git grep 'return E' fs/ | wc -l
 895

For SCSI it's even better, ~97% of the cases use the kernel's standard:

 $ git grep 'return -E' drivers/scsi/ | wc -l
 1448
 $ git grep 'return E' drivers/scsi/ | wc -l
 50

So i'd suggest to make this a default-enabled check. (default disabled 
checks are used only by a small minority) For a _long_ time has this 
been the kernel standard.

	Ingo

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Andy Whitcroft]

On Thu, Nov 12, 2009 at 02:45:54PM +0100, Ingo Molnar wrote:

> So i'd suggest to make this a default-enabled check. (default disabled 
> checks are used only by a small minority) For a _long_ time has this 
> been the kernel standard.

Andrew, I've put a dirty hack in to see how well it stands up against
your incoming flow.  This is in the version at the URL below (it may
take a bit to mirror out):

  http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

-apw

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Dan Merillat]

On Thu, Nov 12, 2009 at 8:45 AM, Ingo Molnar <mingo@elte.hu> wrote:

> Even in filesystems, ~80% of the cases use proper negative values:
>
>  $ git grep 'return -E' fs/ | wc -l
>  4540
>  $ git grep 'return E' fs/ | wc -l
>  895

Except....
fs/9p/fid.c:                            return ERR_PTR(-EPERM);

try this:

$ git grep "return E[A-Z]*;" | grep -v EOF | grep -v ERROR | wc -l
138
$ git grep "return -E[A-Z]*;"| wc -l
57285

2 of those are in Documentation/ and 2 are comments from a quick
glance.   134 uses of positive error returns, _74_ of which are in
fs/xfs, 24 in bluetooth, and the rest scattered randomly around the
kernel.

134 positive error returns  vs 57881 negative?  The style is so
strongly ingrained in the kernel (And I'd bet an audit of those
remaning 134 would find at least one bug) that it'd be a good janitor
task to go through and switch them.


This is one example - a function that returns either ENXIO or 0, and
the lone caller explicitly tests and flips the sign.

(Not signing off on this!!!!  but CCing the relevant people for this driver)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 6399e50..79867d6 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -428,7 +428,7 @@ cciss_proc_write(struct file *file, const char __user *buf,

                rc = cciss_engage_scsi(h->ctlr);
                if (rc != 0)
-                       err = -rc;
+                       err = rc;
                else
                        err = length;
        } else
diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c
index 3315268..4af3085 100644
--- a/drivers/block/cciss_scsi.c
+++ b/drivers/block/cciss_scsi.c
@@ -1547,7 +1547,7 @@ cciss_engage_scsi(int ctlr)
        if (sa->registered) {
                printk("cciss%d: SCSI subsystem already engaged.\n", ctlr);
                spin_unlock_irqrestore(CCISS_LOCK(ctlr), flags);
-               return ENXIO;
+               return -ENXIO;
        }
        sa->registered = 1;
        spin_unlock_irqrestore(CCISS_LOCK(ctlr), flags);

Re: [PATCH] ftrace: return error instead of 12 bytes read

[scameron]

On Wed, Nov 18, 2009 at 05:18:53AM -0500, Dan Merillat wrote:
> On Thu, Nov 12, 2009 at 8:45 AM, Ingo Molnar <mingo@elte.hu> wrote:
> 
> > Even in filesystems, ~80% of the cases use proper negative values:
> >
> >  $ git grep 'return -E' fs/ | wc -l
> >  4540
> >  $ git grep 'return E' fs/ | wc -l
> >  895
> 
> Except....
> fs/9p/fid.c:                            return ERR_PTR(-EPERM);
> 
> try this:
> 
> $ git grep "return E[A-Z]*;" | grep -v EOF | grep -v ERROR | wc -l
> 138
> $ git grep "return -E[A-Z]*;"| wc -l
> 57285
> 
> 2 of those are in Documentation/ and 2 are comments from a quick
> glance.   134 uses of positive error returns, _74_ of which are in
> fs/xfs, 24 in bluetooth, and the rest scattered randomly around the
> kernel.
> 
> 134 positive error returns  vs 57881 negative?  The style is so
> strongly ingrained in the kernel (And I'd bet an audit of those
> remaning 134 would find at least one bug) that it'd be a good janitor
> task to go through and switch them.
> 
> 
> This is one example - a function that returns either ENXIO or 0, and
> the lone caller explicitly tests and flips the sign.
> 
> (Not signing off on this!!!!  but CCing the relevant people for this driver)
> 
> diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> index 6399e50..79867d6 100644
> --- a/drivers/block/cciss.c
> +++ b/drivers/block/cciss.c
> @@ -428,7 +428,7 @@ cciss_proc_write(struct file *file, const char __user *buf,
> 
>                 rc = cciss_engage_scsi(h->ctlr);
>                 if (rc != 0)
> -                       err = -rc;
> +                       err = rc;
>                 else
>                         err = length;
>         } else
> diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c
> index 3315268..4af3085 100644
> --- a/drivers/block/cciss_scsi.c
> +++ b/drivers/block/cciss_scsi.c
> @@ -1547,7 +1547,7 @@ cciss_engage_scsi(int ctlr)
>         if (sa->registered) {
>                 printk("cciss%d: SCSI subsystem already engaged.\n", ctlr);
>                 spin_unlock_irqrestore(CCISS_LOCK(ctlr), flags);
> -               return ENXIO;
> +               return -ENXIO;
>         }
>         sa->registered = 1;
>         spin_unlock_irqrestore(CCISS_LOCK(ctlr), flags);

I sent a patch to fix this already.

http://groups.google.com/group/linux.kernel/browse_thread/thread/40b43d35dff0f30a/0a073fc8014547e0?hl=en&lnk=gst&q=cciss+weird#0a073fc8014547e0

-- steve

Re: [PATCH] ftrace: return error instead of 12 bytes read

[Steven Rostedt]

On Wed, 2009-11-11 at 22:26 +0100, Roel Kluin wrote:
> A negative error value is required: now we cannot
> distinguish ENOMEM from a valid read of 12 bytes.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>

Thanks! I'll pull this in right away and get it out to Ingo.

-- Steve

> ---
>  kernel/trace/trace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index b20d3ec..03c7fd5 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -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);
>  

[PATCH][GIT PULL][v2.6.32] tracing: Fix return value of tracing_stats_read()

[Steven Rostedt]

Ingo,

I know this is late in the rc's, but this is a very minor fix. Do you
think we can slip this in before 33.

Please pull the latest tip/tracing/urgent tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/tracing/urgent


Roel Kluin (1):
      tracing: Fix return value of tracing_stats_read()

----
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
---------------------------
commit a646365cc330b5aaf4452c91f61b1e0d1acf68d0
Author: Roel Kluin <roel.kluin@gmail.com>
Date:   Wed Nov 11 22:26:35 2009 +0100

    tracing: Fix return value of tracing_stats_read()
    
    The function tracing_stats_read() mistakenly returns ENOMEM instead
    of the negative value -ENOMEM.
    
    Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
    LKML-Reference: <4AFB2C0B.50605@gmail.com>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b20d3ec..03c7fd5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -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);
 

Re: [PATCH][GIT PULL][v2.6.32] tracing: Fix return value of tracing_stats_read()

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Ingo,
> 
> I know this is late in the rc's, but this is a very minor fix. Do you
> think we can slip this in before 33.

Yeah, it's an obvious oneliner.

> Please pull the latest tip/tracing/urgent tree, which can be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> tip/tracing/urgent
> 
> 
> Roel Kluin (1):
>       tracing: Fix return value of tracing_stats_read()
> 
> ----
>  kernel/trace/trace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Pulled, thanks!

	Ingo

[tip:tracing/urgent] tracing: Fix return value of tracing_stats_read()

[tip-bot for Roel Kluin]

Commit-ID:  a646365cc330b5aaf4452c91f61b1e0d1acf68d0
Gitweb:     http://git.kernel.org/tip/a646365cc330b5aaf4452c91f61b1e0d1acf68d0
Author:     Roel Kluin <roel.kluin@gmail.com>
AuthorDate: Wed, 11 Nov 2009 22:26:35 +0100
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Wed, 11 Nov 2009 21:26:55 -0500

tracing: Fix return value of tracing_stats_read()

The function tracing_stats_read() mistakenly returns ENOMEM instead
of the negative value -ENOMEM.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
LKML-Reference: <4AFB2C0B.50605@gmail.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b20d3ec..03c7fd5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -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);