[PATCH 01/24] kobject: return actual error on kset_create_and_add

[PATCH 01/24] kobject: return actual error on kset_create_and_add

[Jeff Liu]

From: Jie Liu <jeff.liu@oracle.com>

It's better to return the actual error rather than NULL because
kset_register() can failed due to other reasons.

Cc: Christoph Lameter <cl@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Matt Mackall <mpm@selenic.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <jbacik@fb.com>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: Boaz Harrosh <bharrosh@panasas.com>
Cc: Benny Halevy <bhalevy@primarydata.com>
Cc: Christine Caulfield <ccaulfie@redhat.com>
Cc: David Teigland <teigland@redhat.com>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: Changman Lee <cm224.lee@samsung.com>
Cc: Mark Fasheh <mfasheh@suse.com>
Cc: Joel Becker <jlbec@evilplan.org>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 lib/kobject.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index 58751bb..538c617 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -919,11 +919,11 @@ struct kset *kset_create_and_add(const char *name,
 
 	kset = kset_create(name, uevent_ops, parent_kobj);
 	if (!kset)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	error = kset_register(kset);
 	if (error) {
 		kfree(kset);
-		return NULL;
+		return ERR_PTR(error);
 	}
 	return kset;
 }
-- 
1.8.3.2

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[James Bottomley]

On Tue, 2014-06-17 at 22:25 +0800, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> It's better to return the actual error rather than NULL because
> kset_register() can failed due to other reasons.
> 
> Cc: Christoph Lameter <cl@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Chris Mason <clm@fb.com>
> Cc: Josef Bacik <jbacik@fb.com>
> Cc: Steven Whitehouse <swhiteho@redhat.com>
> Cc: Boaz Harrosh <bharrosh@panasas.com>
> Cc: Benny Halevy <bhalevy@primarydata.com>
> Cc: Christine Caulfield <ccaulfie@redhat.com>
> Cc: David Teigland <teigland@redhat.com>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> Cc: Changman Lee <cm224.lee@samsung.com>
> Cc: Mark Fasheh <mfasheh@suse.com>
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> ---
>  lib/kobject.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 58751bb..538c617 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -919,11 +919,11 @@ struct kset *kset_create_and_add(const char *name,
>  
>  	kset = kset_create(name, uevent_ops, parent_kobj);
>  	if (!kset)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  	error = kset_register(kset);
>  	if (error) {
>  		kfree(kset);
> -		return NULL;
> +		return ERR_PTR(error);
>  	}
>  	return kset;
>  }

This patch series isn't bisectable.  When changing an error signal from
NULL to non-NULL, you have to do it as one patch, otherwise if a
bisection lands in here we'll get error returns from all our kobject
kset code and the kernel won't boot.

It's fine if you're sending it out as 24 patches for review, but it has
to be applied as a single one.

James

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[Jeff Liu]

On 06/17/2014 22:42 PM, James Bottomley wrote:
> On Tue, 2014-06-17 at 22:25 +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@oracle.com>
>>
>> It's better to return the actual error rather than NULL because
>> kset_register() can failed due to other reasons.
>>
>> Cc: Christoph Lameter <cl@linux-foundation.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Pekka Enberg <penberg@kernel.org>
>> Cc: Matt Mackall <mpm@selenic.com>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
>> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: James E.J. Bottomley <JBottomley@parallels.com>
>> Cc: Helge Deller <deller@gmx.de>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Chris Mason <clm@fb.com>
>> Cc: Josef Bacik <jbacik@fb.com>
>> Cc: Steven Whitehouse <swhiteho@redhat.com>
>> Cc: Boaz Harrosh <bharrosh@panasas.com>
>> Cc: Benny Halevy <bhalevy@primarydata.com>
>> Cc: Christine Caulfield <ccaulfie@redhat.com>
>> Cc: David Teigland <teigland@redhat.com>
>> Cc: Theodore Ts'o <tytso@mit.edu>
>> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
>> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
>> Cc: Changman Lee <cm224.lee@samsung.com>
>> Cc: Mark Fasheh <mfasheh@suse.com>
>> Cc: Joel Becker <jlbec@evilplan.org>
>> Cc: Casey Schaufler <casey@schaufler-ca.com>
>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>> ---
>>  lib/kobject.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/kobject.c b/lib/kobject.c
>> index 58751bb..538c617 100644
>> --- a/lib/kobject.c
>> +++ b/lib/kobject.c
>> @@ -919,11 +919,11 @@ struct kset *kset_create_and_add(const char *name,
>>  
>>  	kset = kset_create(name, uevent_ops, parent_kobj);
>>  	if (!kset)
>> -		return NULL;
>> +		return ERR_PTR(-ENOMEM);
>>  	error = kset_register(kset);
>>  	if (error) {
>>  		kfree(kset);
>> -		return NULL;
>> +		return ERR_PTR(error);
>>  	}
>>  	return kset;
>>  }
> 
> This patch series isn't bisectable.  When changing an error signal from
> NULL to non-NULL, you have to do it as one patch, otherwise if a
> bisection lands in here we'll get error returns from all our kobject
> kset code and the kernel won't boot.

Thanks for pointing this out.

> It's fine if you're sending it out as 24 patches for review, but it has
> to be applied as a single one.

Ok, I'll post the v2 patch as a single one once this series has been reviewed.


Cheers,
-Jeff

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[Greg KH]

On Tue, Jun 17, 2014 at 10:56:04PM +0800, Jeff Liu wrote:
> 
> On 06/17/2014 22:42 PM, James Bottomley wrote:
> > On Tue, 2014-06-17 at 22:25 +0800, Jeff Liu wrote:
> >> From: Jie Liu <jeff.liu@oracle.com>
> >>
> >> It's better to return the actual error rather than NULL because
> >> kset_register() can failed due to other reasons.
> >>
> >> Cc: Christoph Lameter <cl@linux-foundation.org>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: Pekka Enberg <penberg@kernel.org>
> >> Cc: Matt Mackall <mpm@selenic.com>
> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> Cc: Paul Mackerras <paulus@samba.org>
> >> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> >> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> >> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> >> Cc: David S. Miller <davem@davemloft.net>
> >> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> >> Cc: Joerg Roedel <joro@8bytes.org>
> >> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> >> Cc: Helge Deller <deller@gmx.de>
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: Chris Mason <clm@fb.com>
> >> Cc: Josef Bacik <jbacik@fb.com>
> >> Cc: Steven Whitehouse <swhiteho@redhat.com>
> >> Cc: Boaz Harrosh <bharrosh@panasas.com>
> >> Cc: Benny Halevy <bhalevy@primarydata.com>
> >> Cc: Christine Caulfield <ccaulfie@redhat.com>
> >> Cc: David Teigland <teigland@redhat.com>
> >> Cc: Theodore Ts'o <tytso@mit.edu>
> >> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> >> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> >> Cc: Changman Lee <cm224.lee@samsung.com>
> >> Cc: Mark Fasheh <mfasheh@suse.com>
> >> Cc: Joel Becker <jlbec@evilplan.org>
> >> Cc: Casey Schaufler <casey@schaufler-ca.com>
> >> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> >> ---
> >>  lib/kobject.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/kobject.c b/lib/kobject.c
> >> index 58751bb..538c617 100644
> >> --- a/lib/kobject.c
> >> +++ b/lib/kobject.c
> >> @@ -919,11 +919,11 @@ struct kset *kset_create_and_add(const char *name,
> >>  
> >>  	kset = kset_create(name, uevent_ops, parent_kobj);
> >>  	if (!kset)
> >> -		return NULL;
> >> +		return ERR_PTR(-ENOMEM);
> >>  	error = kset_register(kset);
> >>  	if (error) {
> >>  		kfree(kset);
> >> -		return NULL;
> >> +		return ERR_PTR(error);
> >>  	}
> >>  	return kset;
> >>  }
> > 
> > This patch series isn't bisectable.  When changing an error signal from
> > NULL to non-NULL, you have to do it as one patch, otherwise if a
> > bisection lands in here we'll get error returns from all our kobject
> > kset code and the kernel won't boot.
> 
> Thanks for pointing this out.
> 
> > It's fine if you're sending it out as 24 patches for review, but it has
> > to be applied as a single one.
> 
> Ok, I'll post the v2 patch as a single one once this series has been reviewed.

Also, now that you sent a bunch of different patches out, some
maintainers will not realize that this first one has to be applied (i.e.
you gave them no sense that there was a dependancy.)

You will have to watch very carefully to ensure that those patches do
not now get accepted, as they will break the existing code.

Not good.

greg k-h

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[Greg KH]

On Tue, Jun 17, 2014 at 08:50:30AM -0700, Greg KH wrote:
> On Tue, Jun 17, 2014 at 10:56:04PM +0800, Jeff Liu wrote:
> > 
> > On 06/17/2014 22:42 PM, James Bottomley wrote:
> > > On Tue, 2014-06-17 at 22:25 +0800, Jeff Liu wrote:
> > >> From: Jie Liu <jeff.liu@oracle.com>
> > >>
> > >> It's better to return the actual error rather than NULL because
> > >> kset_register() can failed due to other reasons.
> > >>
> > >> Cc: Christoph Lameter <cl@linux-foundation.org>
> > >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >> Cc: Andrew Morton <akpm@linux-foundation.org>
> > >> Cc: Pekka Enberg <penberg@kernel.org>
> > >> Cc: Matt Mackall <mpm@selenic.com>
> > >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > >> Cc: Paul Mackerras <paulus@samba.org>
> > >> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > >> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> > >> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > >> Cc: David S. Miller <davem@davemloft.net>
> > >> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> > >> Cc: Joerg Roedel <joro@8bytes.org>
> > >> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> > >> Cc: Helge Deller <deller@gmx.de>
> > >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> > >> Cc: Chris Mason <clm@fb.com>
> > >> Cc: Josef Bacik <jbacik@fb.com>
> > >> Cc: Steven Whitehouse <swhiteho@redhat.com>
> > >> Cc: Boaz Harrosh <bharrosh@panasas.com>
> > >> Cc: Benny Halevy <bhalevy@primarydata.com>
> > >> Cc: Christine Caulfield <ccaulfie@redhat.com>
> > >> Cc: David Teigland <teigland@redhat.com>
> > >> Cc: Theodore Ts'o <tytso@mit.edu>
> > >> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > >> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> > >> Cc: Changman Lee <cm224.lee@samsung.com>
> > >> Cc: Mark Fasheh <mfasheh@suse.com>
> > >> Cc: Joel Becker <jlbec@evilplan.org>
> > >> Cc: Casey Schaufler <casey@schaufler-ca.com>
> > >> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> > >> ---
> > >>  lib/kobject.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/lib/kobject.c b/lib/kobject.c
> > >> index 58751bb..538c617 100644
> > >> --- a/lib/kobject.c
> > >> +++ b/lib/kobject.c
> > >> @@ -919,11 +919,11 @@ struct kset *kset_create_and_add(const char *name,
> > >>  
> > >>  	kset = kset_create(name, uevent_ops, parent_kobj);
> > >>  	if (!kset)
> > >> -		return NULL;
> > >> +		return ERR_PTR(-ENOMEM);
> > >>  	error = kset_register(kset);
> > >>  	if (error) {
> > >>  		kfree(kset);
> > >> -		return NULL;
> > >> +		return ERR_PTR(error);
> > >>  	}
> > >>  	return kset;
> > >>  }
> > > 
> > > This patch series isn't bisectable.  When changing an error signal from
> > > NULL to non-NULL, you have to do it as one patch, otherwise if a
> > > bisection lands in here we'll get error returns from all our kobject
> > > kset code and the kernel won't boot.
> > 
> > Thanks for pointing this out.
> > 
> > > It's fine if you're sending it out as 24 patches for review, but it has
> > > to be applied as a single one.
> > 
> > Ok, I'll post the v2 patch as a single one once this series has been reviewed.
> 
> Also, now that you sent a bunch of different patches out, some
> maintainers will not realize that this first one has to be applied (i.e.
> you gave them no sense that there was a dependancy.)
> 
> You will have to watch very carefully to ensure that those patches do
> not now get accepted, as they will break the existing code.

I've emailed a few responses to the patches that I could find, but
please follow-up and email everyone to not apply the patches as
responses to those patches so that everyone is properly notified.

greg k-h

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[Jeff Liu]

On 06/17/2014 23:50 PM, Greg KH wrote:
> On Tue, Jun 17, 2014 at 10:56:04PM +0800, Jeff Liu wrote:
>>
>> On 06/17/2014 22:42 PM, James Bottomley wrote:
>>> On Tue, 2014-06-17 at 22:25 +0800, Jeff Liu wrote:
>>>> From: Jie Liu <jeff.liu@oracle.com>
>>>>
>>>> It's better to return the actual error rather than NULL because
>>>> kset_register() can failed due to other reasons.
>>>>

<snip>

>>>> 
>>>
>>> This patch series isn't bisectable.  When changing an error signal from
>>> NULL to non-NULL, you have to do it as one patch, otherwise if a
>>> bisection lands in here we'll get error returns from all our kobject
>>> kset code and the kernel won't boot.
>>
>> Thanks for pointing this out.
>>
>>> It's fine if you're sending it out as 24 patches for review, but it has
>>> to be applied as a single one.
>>
>> Ok, I'll post the v2 patch as a single one once this series has been reviewed.
> 
> Also, now that you sent a bunch of different patches out, some
> maintainers will not realize that this first one has to be applied (i.e.
> you gave them no sense that there was a dependancy.)
> 
> You will have to watch very carefully to ensure that those patches do
> not now get accepted, as they will break the existing code.

Ok, and I'll inform every other maintainers do not apply their patch individually.


Cheers,
-Jeff

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[Greg KH]

On Tue, Jun 17, 2014 at 10:25:59PM +0800, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> It's better to return the actual error rather than NULL because
> kset_register() can failed due to other reasons.

Why?

Who really cares about that?  What can do you do with that additional
information that will help anything out?

In other words, what benifit does this patch create to justify the large
churn it is going to cause?

thanks,

greg k-h

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[Christoph Lameter]

On Tue, 17 Jun 2014, Greg KH wrote:

> Who really cares about that?  What can do you do with that additional
> information that will help anything out?

The obvious reason for doing so is so that one can put the error code into
a log message to get more detailed information on why the call failed.

In this particular case the slab allocator can give an error message that
gives a hint why the sysfs support could not be enabled.

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[James Bottomley]

On Tue, 2014-06-17 at 11:00 -0500, Christoph Lameter wrote:
> On Tue, 17 Jun 2014, Greg KH wrote:
> 
> > Who really cares about that?  What can do you do with that additional
> > information that will help anything out?
> 
> The obvious reason for doing so is so that one can put the error code into
> a log message to get more detailed information on why the call failed.
> 
> In this particular case the slab allocator can give an error message that
> gives a hint why the sysfs support could not be enabled.

What useful information does this supply?  The current assumption on
NULL return is -ENOMEM ... what's the interesting case where this
assumption is wrong?

James

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[Christoph Lameter]

On Tue, 17 Jun 2014, James Bottomley wrote:

> What useful information does this supply?  The current assumption on
> NULL return is -ENOMEM ... what's the interesting case where this
> assumption is wrong?

No idea. Sysfs just ignored all error codes and returns NULL right now.
If it is indeed just ENOMEN then we have no issue.

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[James Bottomley]

On Tue, 2014-06-17 at 11:29 -0500, Christoph Lameter wrote:
> On Tue, 17 Jun 2014, James Bottomley wrote:
> 
> > What useful information does this supply?  The current assumption on
> > NULL return is -ENOMEM ... what's the interesting case where this
> > assumption is wrong?
> 
> No idea. Sysfs just ignored all error codes and returns NULL right now.
> If it is indeed just ENOMEN then we have no issue.

Well, I think when we have that justification, then we can decide on the
merits of the patch set.

James

Re: [PATCH 01/24] kobject: return actual error on kset_create_and_add

[Greg KH]

On Tue, Jun 17, 2014 at 09:54:49AM -0700, James Bottomley wrote:
> On Tue, 2014-06-17 at 11:29 -0500, Christoph Lameter wrote:
> > On Tue, 17 Jun 2014, James Bottomley wrote:
> > 
> > > What useful information does this supply?  The current assumption on
> > > NULL return is -ENOMEM ... what's the interesting case where this
> > > assumption is wrong?
> > 
> > No idea. Sysfs just ignored all error codes and returns NULL right now.
> > If it is indeed just ENOMEN then we have no issue.
> 
> Well, I think when we have that justification, then we can decide on the
> merits of the patch set.

There is one error patch in this function that is not due to a -ENOMEM
error, but that will generate a huge message in the syslog describing
what happened (created with the same name, or some other reason), so it
would not make sense to duplicate another error message in the log by
the caller code.

So again, I don't see any use for this patch at all, except to cause
problems (mix of maintainers applying the follow-on patches and others
not).

Everyone, please do not apply any of these patches that were sent to
you in this series.

thanks,

greg k-h