commit ff9fb72bc077 ("debugfs: return error values, not NULL") changed
the return values of various debugfs functions in fs/debugfs/inode.c but
did not update the documentation or behaviour of affected functions in
fs/debugfs/file.c . These patches attempt to remedy this.
Ronald Tschalär (2):
debugfs: update documented return values of debugfs helpers
debugfs: make return value of all debugfs helpers consistent
Documentation/filesystems/debugfs.txt | 16 +++---
fs/debugfs/file.c | 79 ++++++++++++---------------
2 files changed, 45 insertions(+), 50 deletions(-)
--
2.20.1
Since commit ff9fb72bc077 ("debugfs: return error values, not NULL")
these helper functions do not return NULL anymore (with the exception
of debugfs_create_u32_array()).
Signed-off-by: Ronald Tschalär <[email protected]>
---
Documentation/filesystems/debugfs.txt | 16 +++---
fs/debugfs/file.c | 77 ++++++++++++---------------
2 files changed, 44 insertions(+), 49 deletions(-)
diff --git a/Documentation/filesystems/debugfs.txt b/Documentation/filesystems/debugfs.txt
index 4f45f71149cb..4a0a9c3f4af6 100644
--- a/Documentation/filesystems/debugfs.txt
+++ b/Documentation/filesystems/debugfs.txt
@@ -31,10 +31,10 @@ This call, if successful, will make a directory called name underneath the
indicated parent directory. If parent is NULL, the directory will be
created in the debugfs root. On success, the return value is a struct
dentry pointer which can be used to create files in the directory (and to
-clean it up at the end). A NULL return value indicates that something went
-wrong. If ERR_PTR(-ENODEV) is returned, that is an indication that the
-kernel has been built without debugfs support and none of the functions
-described below will work.
+clean it up at the end). An ERR_PTR(-ERROR) return value indicates that
+something went wrong. If ERR_PTR(-ENODEV) is returned, that is an
+indication that the kernel has been built without debugfs support and none
+of the functions described below will work.
The most general way to create a file within a debugfs directory is with:
@@ -48,8 +48,9 @@ should hold the file, data will be stored in the i_private field of the
resulting inode structure, and fops is a set of file operations which
implement the file's behavior. At a minimum, the read() and/or write()
operations should be provided; others can be included as needed. Again,
-the return value will be a dentry pointer to the created file, NULL for
-error, or ERR_PTR(-ENODEV) if debugfs support is missing.
+the return value will be a dentry pointer to the created file,
+ERR_PTR(-ERROR) on error, or ERR_PTR(-ENODEV) if debugfs support is
+missing.
Create a file with an initial size, the following function can be used
instead:
@@ -214,7 +215,8 @@ can be removed with:
void debugfs_remove(struct dentry *dentry);
-The dentry value can be NULL, in which case nothing will be removed.
+The dentry value can be NULL or an error value, in which case nothing will
+be removed.
Once upon a time, debugfs users were required to remember the dentry
pointer for every debugfs file they created so that all files could be
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 4fce1da7db23..ddd708b09fa1 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -394,12 +394,11 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_u8_wo, NULL, debugfs_u8_set, "%llu\n");
* This function will return a pointer to a dentry if it succeeds. This
* pointer must be passed to the debugfs_remove() function when the file is
* to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.) If an error occurs, %NULL will be returned.
+ * you are responsible here.) If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
*
- * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned. It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * If debugfs is not enabled in the kernel, the value %ERR_PTR(-ENODEV) will
+ * be returned.
*/
struct dentry *debugfs_create_u8(const char *name, umode_t mode,
struct dentry *parent, u8 *value)
@@ -440,12 +439,11 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_u16_wo, NULL, debugfs_u16_set, "%llu\n");
* This function will return a pointer to a dentry if it succeeds. This
* pointer must be passed to the debugfs_remove() function when the file is
* to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.) If an error occurs, %NULL will be returned.
+ * you are responsible here.) If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
*
- * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned. It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * If debugfs is not enabled in the kernel, the value %ERR_PTR(-ENODEV) will
+ * be returned.
*/
struct dentry *debugfs_create_u16(const char *name, umode_t mode,
struct dentry *parent, u16 *value)
@@ -486,12 +484,11 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_u32_wo, NULL, debugfs_u32_set, "%llu\n");
* This function will return a pointer to a dentry if it succeeds. This
* pointer must be passed to the debugfs_remove() function when the file is
* to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.) If an error occurs, %NULL will be returned.
+ * you are responsible here.) If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
*
- * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned. It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * If debugfs is not enabled in the kernel, the value %ERR_PTR(-ENODEV) will
+ * be returned.
*/
struct dentry *debugfs_create_u32(const char *name, umode_t mode,
struct dentry *parent, u32 *value)
@@ -533,12 +530,11 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_u64_wo, NULL, debugfs_u64_set, "%llu\n");
* This function will return a pointer to a dentry if it succeeds. This
* pointer must be passed to the debugfs_remove() function when the file is
* to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.) If an error occurs, %NULL will be returned.
+ * you are responsible here.) If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
*
- * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned. It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * If debugfs is not enabled in the kernel, the value %ERR_PTR(-ENODEV) will
+ * be returned.
*/
struct dentry *debugfs_create_u64(const char *name, umode_t mode,
struct dentry *parent, u64 *value)
@@ -582,12 +578,11 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ulong_wo, NULL, debugfs_ulong_set, "%llu\n");
* This function will return a pointer to a dentry if it succeeds. This
* pointer must be passed to the debugfs_remove() function when the file is
* to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.) If an error occurs, %NULL will be returned.
+ * you are responsible here.) If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
*
- * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned. It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * If debugfs is not enabled in the kernel, the value %ERR_PTR(-ENODEV) will
+ * be returned.
*/
struct dentry *debugfs_create_ulong(const char *name, umode_t mode,
struct dentry *parent, unsigned long *value)
@@ -850,12 +845,11 @@ static const struct file_operations fops_bool_wo = {
* This function will return a pointer to a dentry if it succeeds. This
* pointer must be passed to the debugfs_remove() function when the file is
* to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.) If an error occurs, %NULL will be returned.
+ * you are responsible here.) If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
*
- * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned. It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * If debugfs is not enabled in the kernel, the value %ERR_PTR(-ENODEV) will
+ * be returned.
*/
struct dentry *debugfs_create_bool(const char *name, umode_t mode,
struct dentry *parent, bool *value)
@@ -904,12 +898,11 @@ static const struct file_operations fops_blob = {
* This function will return a pointer to a dentry if it succeeds. This
* pointer must be passed to the debugfs_remove() function when the file is
* to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.) If an error occurs, %NULL will be returned.
+ * you are responsible here.) If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
*
- * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned. It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * If debugfs is not enabled in the kernel, the value %ERR_PTR(-ENODEV) will
+ * be returned.
*/
struct dentry *debugfs_create_blob(const char *name, umode_t mode,
struct dentry *parent,
@@ -1005,8 +998,9 @@ static const struct file_operations u32_array_fops = {
* Writing is not supported. Seek within the file is also not supported.
* Once array is created its size can not be changed.
*
- * The function returns a pointer to dentry on success. If debugfs is not
- * enabled in the kernel, the value -%ENODEV will be returned.
+ * The function returns a pointer to dentry on success. If an error occurs,
+ * %ERR_PTR(-ERROR) or NULL will be returned. If debugfs is not enabled in
+ * the kernel, the value %ERR_PTR(-ENODEV) will be returned.
*/
struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
struct dentry *parent,
@@ -1102,12 +1096,11 @@ static const struct file_operations fops_regset32 = {
* This function will return a pointer to a dentry if it succeeds. This
* pointer must be passed to the debugfs_remove() function when the file is
* to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.) If an error occurs, %NULL will be returned.
+ * you are responsible here.) If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
*
- * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned. It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * If debugfs is not enabled in the kernel, the value %ERR_PTR(-ENODEV) will
+ * be returned.
*/
struct dentry *debugfs_create_regset32(const char *name, umode_t mode,
struct dentry *parent,
--
2.20.1
Since commit ff9fb72bc077 ("debugfs: return error values, not NULL")
almost all the debugfs helpers have stopped returning NULL. The lone
holdeout was debugfs_create_u32_array(). So fix that.
Signed-off-by: Ronald Tschalär <[email protected]>
---
fs/debugfs/file.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index ddd708b09fa1..bb706d073782 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -999,8 +999,8 @@ static const struct file_operations u32_array_fops = {
* Once array is created its size can not be changed.
*
* The function returns a pointer to dentry on success. If an error occurs,
- * %ERR_PTR(-ERROR) or NULL will be returned. If debugfs is not enabled in
- * the kernel, the value %ERR_PTR(-ENODEV) will be returned.
+ * %ERR_PTR(-ERROR) will be returned. If debugfs is not enabled in the kernel,
+ * the value %ERR_PTR(-ENODEV) will be returned.
*/
struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
struct dentry *parent,
@@ -1009,7 +1009,7 @@ struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
struct array_data *data = kmalloc(sizeof(*data), GFP_KERNEL);
if (data == NULL)
- return NULL;
+ return ERR_PTR(-ENOMEM);
data->array = array;
data->elements = elements;
--
2.20.1
On Mon, Apr 15, 2019 at 01:25:06AM -0700, Ronald Tschal?r wrote:
> Since commit ff9fb72bc077 ("debugfs: return error values, not NULL")
> almost all the debugfs helpers have stopped returning NULL. The lone
> holdeout was debugfs_create_u32_array(). So fix that.
>
> Signed-off-by: Ronald Tschal?r <[email protected]>
> ---
> fs/debugfs/file.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index ddd708b09fa1..bb706d073782 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -999,8 +999,8 @@ static const struct file_operations u32_array_fops = {
> * Once array is created its size can not be changed.
> *
> * The function returns a pointer to dentry on success. If an error occurs,
> - * %ERR_PTR(-ERROR) or NULL will be returned. If debugfs is not enabled in
> - * the kernel, the value %ERR_PTR(-ENODEV) will be returned.
> + * %ERR_PTR(-ERROR) will be returned. If debugfs is not enabled in the kernel,
> + * the value %ERR_PTR(-ENODEV) will be returned.
> */
> struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
> struct dentry *parent,
> @@ -1009,7 +1009,7 @@ struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
> struct array_data *data = kmalloc(sizeof(*data), GFP_KERNEL);
>
> if (data == NULL)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> data->array = array;
> data->elements = elements;
There is only one caller of this function in the kernel now, and it does
not even care about the return value at all, so we should just remove
the return value entirely as that's the easiest and best thing to do
here. I was going to start doing this slowly over time, but as you are
touching the function now, might as well do it here :)
thanks,
greg k-h
Hi Greg,
On Mon, Apr 15, 2019 at 10:48:44AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 15, 2019 at 01:25:06AM -0700, Ronald Tschal?r wrote:
> > Since commit ff9fb72bc077 ("debugfs: return error values, not NULL")
> > almost all the debugfs helpers have stopped returning NULL. The lone
> > holdeout was debugfs_create_u32_array(). So fix that.
> >
> > Signed-off-by: Ronald Tschal?r <[email protected]>
> > ---
> > fs/debugfs/file.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > index ddd708b09fa1..bb706d073782 100644
> > --- a/fs/debugfs/file.c
> > +++ b/fs/debugfs/file.c
> > @@ -999,8 +999,8 @@ static const struct file_operations u32_array_fops = {
> > * Once array is created its size can not be changed.
> > *
> > * The function returns a pointer to dentry on success. If an error occurs,
> > - * %ERR_PTR(-ERROR) or NULL will be returned. If debugfs is not enabled in
> > - * the kernel, the value %ERR_PTR(-ENODEV) will be returned.
> > + * %ERR_PTR(-ERROR) will be returned. If debugfs is not enabled in the kernel,
> > + * the value %ERR_PTR(-ENODEV) will be returned.
> > */
> > struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
> > struct dentry *parent,
> > @@ -1009,7 +1009,7 @@ struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
> > struct array_data *data = kmalloc(sizeof(*data), GFP_KERNEL);
> >
> > if (data == NULL)
> > - return NULL;
> > + return ERR_PTR(-ENOMEM);
> >
> > data->array = array;
> > data->elements = elements;
>
> There is only one caller of this function in the kernel now, and it does
> not even care about the return value at all, so we should just remove
> the return value entirely as that's the easiest and best thing to do
> here.
Interesting argument: since this is a helper/library function, and
therefore potentially used in the future by others, it seems to me
that consistency with the other functions and providing error feedback
would be important.
> I was going to start doing this slowly over time, but as you are
> touching the function now, might as well do it here :)
Are you saying the plan is to make all these helpers return void?
Cheers,
Ronald
On Mon, Apr 15, 2019 at 04:29:59PM -0700, Life is hard, and then you die wrote:
>
> Hi Greg,
>
> On Mon, Apr 15, 2019 at 10:48:44AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Apr 15, 2019 at 01:25:06AM -0700, Ronald Tschal?r wrote:
> > > Since commit ff9fb72bc077 ("debugfs: return error values, not NULL")
> > > almost all the debugfs helpers have stopped returning NULL. The lone
> > > holdeout was debugfs_create_u32_array(). So fix that.
> > >
> > > Signed-off-by: Ronald Tschal?r <[email protected]>
> > > ---
> > > fs/debugfs/file.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > > index ddd708b09fa1..bb706d073782 100644
> > > --- a/fs/debugfs/file.c
> > > +++ b/fs/debugfs/file.c
> > > @@ -999,8 +999,8 @@ static const struct file_operations u32_array_fops = {
> > > * Once array is created its size can not be changed.
> > > *
> > > * The function returns a pointer to dentry on success. If an error occurs,
> > > - * %ERR_PTR(-ERROR) or NULL will be returned. If debugfs is not enabled in
> > > - * the kernel, the value %ERR_PTR(-ENODEV) will be returned.
> > > + * %ERR_PTR(-ERROR) will be returned. If debugfs is not enabled in the kernel,
> > > + * the value %ERR_PTR(-ENODEV) will be returned.
> > > */
> > > struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
> > > struct dentry *parent,
> > > @@ -1009,7 +1009,7 @@ struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
> > > struct array_data *data = kmalloc(sizeof(*data), GFP_KERNEL);
> > >
> > > if (data == NULL)
> > > - return NULL;
> > > + return ERR_PTR(-ENOMEM);
> > >
> > > data->array = array;
> > > data->elements = elements;
> >
> > There is only one caller of this function in the kernel now, and it does
> > not even care about the return value at all, so we should just remove
> > the return value entirely as that's the easiest and best thing to do
> > here.
>
> Interesting argument: since this is a helper/library function, and
> therefore potentially used in the future by others, it seems to me
> that consistency with the other functions and providing error feedback
> would be important.
Not if you never actually use the return value for anything :)
> > I was going to start doing this slowly over time, but as you are
> > touching the function now, might as well do it here :)
>
> Are you saying the plan is to make all these helpers return void?
Yes, no caller should do anything "different" based on a return value of
a debugfs call. Note, sometimes you do want to save off dentries for
some files that you later remove, but that's it.
thanks,
greg k-h
The single user of debugfs_create_u32_array() does not care about the
return value of it, so make it return void as there is no need to do
anything with the return value.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
Documentation/filesystems/debugfs.txt | 2 +-
fs/debugfs/file.c | 13 ++++---------
include/linux/debugfs.h | 12 +++++-------
3 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/Documentation/filesystems/debugfs.txt b/Documentation/filesystems/debugfs.txt
index 4f45f71149cb..6320e86d2294 100644
--- a/Documentation/filesystems/debugfs.txt
+++ b/Documentation/filesystems/debugfs.txt
@@ -168,7 +168,7 @@ byte offsets over a base for the register block.
If you want to dump an u32 array in debugfs, you can create file with:
- struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
+ void debugfs_create_u32_array(const char *name, umode_t mode,
struct dentry *parent,
u32 *array, u32 elements);
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 4fce1da7db23..2c17039c6287 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1004,24 +1004,19 @@ static const struct file_operations u32_array_fops = {
* @array as data. If the @mode variable is so set it can be read from.
* Writing is not supported. Seek within the file is also not supported.
* Once array is created its size can not be changed.
- *
- * The function returns a pointer to dentry on success. If debugfs is not
- * enabled in the kernel, the value -%ENODEV will be returned.
*/
-struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
- struct dentry *parent,
- u32 *array, u32 elements)
+void debugfs_create_u32_array(const char *name, umode_t mode,
+ struct dentry *parent, u32 *array, u32 elements)
{
struct array_data *data = kmalloc(sizeof(*data), GFP_KERNEL);
if (data == NULL)
- return NULL;
+ return;
data->array = array;
data->elements = elements;
- return debugfs_create_file_unsafe(name, mode, parent, data,
- &u32_array_fops);
+ debugfs_create_file_unsafe(name, mode, parent, data, &u32_array_fops);
}
EXPORT_SYMBOL_GPL(debugfs_create_u32_array);
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 3b0ba54cc4d5..58424eb3b329 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -133,9 +133,8 @@ struct dentry *debugfs_create_regset32(const char *name, umode_t mode,
void debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
int nregs, void __iomem *base, char *prefix);
-struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
- struct dentry *parent,
- u32 *array, u32 elements);
+void debugfs_create_u32_array(const char *name, umode_t mode,
+ struct dentry *parent, u32 *array, u32 elements);
struct dentry *debugfs_create_devm_seqfile(struct device *dev, const char *name,
struct dentry *parent,
@@ -353,11 +352,10 @@ static inline bool debugfs_initialized(void)
return false;
}
-static inline struct dentry *debugfs_create_u32_array(const char *name, umode_t mode,
- struct dentry *parent,
- u32 *array, u32 elements)
+static inline void debugfs_create_u32_array(const char *name, umode_t mode,
+ struct dentry *parent, u32 *array,
+ u32 elements)
{
- return ERR_PTR(-ENODEV);
}
static inline struct dentry *debugfs_create_devm_seqfile(struct device *dev,
--
2.21.0