From: Jie Liu <[email protected]>
It's better to return the actual error rather than NULL because
kset_register() can failed due to other reasons.
Cc: Christoph Lameter <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: Matt Mackall <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Martin Schwidefsky <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Stefan Richter <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: James E.J. Bottomley <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Chris Mason <[email protected]>
Cc: Josef Bacik <[email protected]>
Cc: Steven Whitehouse <[email protected]>
Cc: Boaz Harrosh <[email protected]>
Cc: Benny Halevy <[email protected]>
Cc: Christine Caulfield <[email protected]>
Cc: David Teigland <[email protected]>
Cc: Theodore Ts'o <[email protected]>
Cc: Andreas Dilger <[email protected]>
Cc: Jaegeuk Kim <[email protected]>
Cc: Changman Lee <[email protected]>
Cc: Mark Fasheh <[email protected]>
Cc: Joel Becker <[email protected]>
Cc: Casey Schaufler <[email protected]>
Signed-off-by: Jie Liu <[email protected]>
---
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
On Tue, 2014-06-17 at 22:25 +0800, Jeff Liu wrote:
> From: Jie Liu <[email protected]>
>
> It's better to return the actual error rather than NULL because
> kset_register() can failed due to other reasons.
>
> Cc: Christoph Lameter <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Cc: Matt Mackall <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Martin Schwidefsky <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Stefan Richter <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: James E.J. Bottomley <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Chris Mason <[email protected]>
> Cc: Josef Bacik <[email protected]>
> Cc: Steven Whitehouse <[email protected]>
> Cc: Boaz Harrosh <[email protected]>
> Cc: Benny Halevy <[email protected]>
> Cc: Christine Caulfield <[email protected]>
> Cc: David Teigland <[email protected]>
> Cc: Theodore Ts'o <[email protected]>
> Cc: Andreas Dilger <[email protected]>
> Cc: Jaegeuk Kim <[email protected]>
> Cc: Changman Lee <[email protected]>
> Cc: Mark Fasheh <[email protected]>
> Cc: Joel Becker <[email protected]>
> Cc: Casey Schaufler <[email protected]>
> Signed-off-by: Jie Liu <[email protected]>
> ---
> 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
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 <[email protected]>
>>
>> It's better to return the actual error rather than NULL because
>> kset_register() can failed due to other reasons.
>>
>> Cc: Christoph Lameter <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Pekka Enberg <[email protected]>
>> Cc: Matt Mackall <[email protected]>
>> Cc: Benjamin Herrenschmidt <[email protected]>
>> Cc: Paul Mackerras <[email protected]>
>> Cc: Martin Schwidefsky <[email protected]>
>> Cc: Heiko Carstens <[email protected]>
>> Cc: Herbert Xu <[email protected]>
>> Cc: David S. Miller <[email protected]>
>> Cc: Stefan Richter <[email protected]>
>> Cc: Joerg Roedel <[email protected]>
>> Cc: James E.J. Bottomley <[email protected]>
>> Cc: Helge Deller <[email protected]>
>> Cc: Bjorn Helgaas <[email protected]>
>> Cc: Chris Mason <[email protected]>
>> Cc: Josef Bacik <[email protected]>
>> Cc: Steven Whitehouse <[email protected]>
>> Cc: Boaz Harrosh <[email protected]>
>> Cc: Benny Halevy <[email protected]>
>> Cc: Christine Caulfield <[email protected]>
>> Cc: David Teigland <[email protected]>
>> Cc: Theodore Ts'o <[email protected]>
>> Cc: Andreas Dilger <[email protected]>
>> Cc: Jaegeuk Kim <[email protected]>
>> Cc: Changman Lee <[email protected]>
>> Cc: Mark Fasheh <[email protected]>
>> Cc: Joel Becker <[email protected]>
>> Cc: Casey Schaufler <[email protected]>
>> Signed-off-by: Jie Liu <[email protected]>
>> ---
>> 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
On Tue, Jun 17, 2014 at 10:25:59PM +0800, Jeff Liu wrote:
> From: Jie Liu <[email protected]>
>
> 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
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 <[email protected]>
> >>
> >> It's better to return the actual error rather than NULL because
> >> kset_register() can failed due to other reasons.
> >>
> >> Cc: Christoph Lameter <[email protected]>
> >> Cc: Greg Kroah-Hartman <[email protected]>
> >> Cc: Andrew Morton <[email protected]>
> >> Cc: Pekka Enberg <[email protected]>
> >> Cc: Matt Mackall <[email protected]>
> >> Cc: Benjamin Herrenschmidt <[email protected]>
> >> Cc: Paul Mackerras <[email protected]>
> >> Cc: Martin Schwidefsky <[email protected]>
> >> Cc: Heiko Carstens <[email protected]>
> >> Cc: Herbert Xu <[email protected]>
> >> Cc: David S. Miller <[email protected]>
> >> Cc: Stefan Richter <[email protected]>
> >> Cc: Joerg Roedel <[email protected]>
> >> Cc: James E.J. Bottomley <[email protected]>
> >> Cc: Helge Deller <[email protected]>
> >> Cc: Bjorn Helgaas <[email protected]>
> >> Cc: Chris Mason <[email protected]>
> >> Cc: Josef Bacik <[email protected]>
> >> Cc: Steven Whitehouse <[email protected]>
> >> Cc: Boaz Harrosh <[email protected]>
> >> Cc: Benny Halevy <[email protected]>
> >> Cc: Christine Caulfield <[email protected]>
> >> Cc: David Teigland <[email protected]>
> >> Cc: Theodore Ts'o <[email protected]>
> >> Cc: Andreas Dilger <[email protected]>
> >> Cc: Jaegeuk Kim <[email protected]>
> >> Cc: Changman Lee <[email protected]>
> >> Cc: Mark Fasheh <[email protected]>
> >> Cc: Joel Becker <[email protected]>
> >> Cc: Casey Schaufler <[email protected]>
> >> Signed-off-by: Jie Liu <[email protected]>
> >> ---
> >> 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
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.
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
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.
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
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
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 <[email protected]>
> > >>
> > >> It's better to return the actual error rather than NULL because
> > >> kset_register() can failed due to other reasons.
> > >>
> > >> Cc: Christoph Lameter <[email protected]>
> > >> Cc: Greg Kroah-Hartman <[email protected]>
> > >> Cc: Andrew Morton <[email protected]>
> > >> Cc: Pekka Enberg <[email protected]>
> > >> Cc: Matt Mackall <[email protected]>
> > >> Cc: Benjamin Herrenschmidt <[email protected]>
> > >> Cc: Paul Mackerras <[email protected]>
> > >> Cc: Martin Schwidefsky <[email protected]>
> > >> Cc: Heiko Carstens <[email protected]>
> > >> Cc: Herbert Xu <[email protected]>
> > >> Cc: David S. Miller <[email protected]>
> > >> Cc: Stefan Richter <[email protected]>
> > >> Cc: Joerg Roedel <[email protected]>
> > >> Cc: James E.J. Bottomley <[email protected]>
> > >> Cc: Helge Deller <[email protected]>
> > >> Cc: Bjorn Helgaas <[email protected]>
> > >> Cc: Chris Mason <[email protected]>
> > >> Cc: Josef Bacik <[email protected]>
> > >> Cc: Steven Whitehouse <[email protected]>
> > >> Cc: Boaz Harrosh <[email protected]>
> > >> Cc: Benny Halevy <[email protected]>
> > >> Cc: Christine Caulfield <[email protected]>
> > >> Cc: David Teigland <[email protected]>
> > >> Cc: Theodore Ts'o <[email protected]>
> > >> Cc: Andreas Dilger <[email protected]>
> > >> Cc: Jaegeuk Kim <[email protected]>
> > >> Cc: Changman Lee <[email protected]>
> > >> Cc: Mark Fasheh <[email protected]>
> > >> Cc: Joel Becker <[email protected]>
> > >> Cc: Casey Schaufler <[email protected]>
> > >> Signed-off-by: Jie Liu <[email protected]>
> > >> ---
> > >> 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
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 <[email protected]>
>>>>
>>>> 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