kobject: return actual error on kset_create_and_add

PATCH Applied Jeff Liu 4 participants 12 messages Jun 17, 2014 – Jun 18, 2014

Subsystems (2)

Library code Driver core

lore

Share & tools

lore

Jeff Liu

author

2014-06-17 14:28 UTC #0 lore raw

PATCH 1/24 kobject: return actual error on kset_create_and_add

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

lib/kobject.c +2 -2

index 58751bb..538c617 100644
@@ -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;
 }

Signature · 1 line

1.8.3.2

James Bottomley

2014-06-17 14:42 UTC #1 lore raw

Quoted 48 lines · Jeff Liu

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(-)
lib/kobject.c +0 -0
index 58751bb..538c617 100644
@@ -919,11 +919,11 @@ struct kset *kset_create_and_add(const char *name,
 
 	kset = kset_create(name, uevent_ops, parent_kobj);
 	if (!kset)

lib/kobject.c struct kset *kset_create_and_add(const char *name, · line 929

Quoted 10 lines

lib/kobject.c +2 -2

-		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

Jeff Liu

author

2014-06-17 14:59 UTC #2 lore raw

Quoted 56 lines · James Bottomley

Quoted 55 lines · Jeff Liu
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(-)
lib/kobject.c +2 -2
index 58751bb..538c617 100644
@@ -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);

Quoted 8 lines

lib/kobject.c struct kset *kset_create_and_add(const char *name, · line 929
Quoted 3 lines
lib/kobject.c +0 -0
 	}
 	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.

Quoted 2 lines

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

Greg KH

maintainer

2014-06-17 15:46 UTC #3 lore raw

Quoted 67 lines · Jeff Liu

Quoted 56 lines · James Bottomley
Quoted 55 lines · Jeff Liu
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(-)
lib/kobject.c +2 -2
index 58751bb..538c617 100644
@@ -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);
Quoted 8 lines
lib/kobject.c struct kset *kset_create_and_add(const char *name, · line 929
Quoted 3 lines
lib/kobject.c +0 -0
 	}
 	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.

Quoted 6 lines

Thanks for pointing this out.
Quoted 2 lines
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

Greg KH

maintainer

2014-06-17 19:22 UTC #4 lore raw

Quoted 73 lines · Greg KH

Quoted 72 lines · Jeff Liu
Quoted 56 lines · James Bottomley
Quoted 55 lines · Jeff Liu
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(-)
lib/kobject.c +2 -2
index 58751bb..538c617 100644
@@ -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);
Quoted 8 lines
lib/kobject.c struct kset *kset_create_and_add(const char *name, · line 929
Quoted 3 lines
lib/kobject.c +0 -0
 	}
 	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.
Quoted 2 lines
It's fine if you're sending it out as 24 patches for review, but it has to be applied as a single one.

Quoted 8 lines

Quoted 1 line
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

Jeff Liu

author

2014-06-18 01:42 UTC #5 lore raw

In reply to Greg KH

Quoted 9 lines · Greg KH

Quoted 8 lines · Jeff Liu
Quoted 6 lines · James Bottomley
Quoted 5 lines · Jeff Liu
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>

Quoted 12 lines

Quoted 12 lines
Quoted 6 lines
Quoted 1 line
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.
Quoted 2 lines
It's fine if you're sending it out as 24 patches for review, but it has to be applied as a single one.

Quoted 8 lines

Quoted 1 line
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

Greg KH

maintainer

2014-06-17 15:43 UTC #6 lore raw

In reply to Jeff Liu

Quoted 4 lines · Jeff Liu

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

Christoph Lameter

2014-06-17 16:00 UTC #7 lore raw

Quoted 2 lines · Greg KH

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.

James Bottomley

2014-06-17 16:09 UTC #8 lore raw

Quoted 10 lines · Christoph Lameter

Quoted 2 lines · Greg KH
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

Christoph Lameter

2014-06-17 16:29 UTC #9 lore raw

Quoted 3 lines · James Bottomley

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.

James Bottomley

2014-06-17 16:54 UTC #10 lore raw

Quoted 8 lines · Christoph Lameter

Quoted 3 lines · James Bottomley
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

Greg KH

maintainer

2014-06-17 19:18 UTC #11 lore raw

Quoted 7 lines · James Bottomley

Quoted 6 lines · Christoph Lameter
Quoted 3 lines · James Bottomley
What useful information does this supply? The current assumption on NULL return is -ENOMEM ... what's the interesting case where this assumption is wrong?

Quoted 5 lines

Quoted 2 lines
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