[RFC] debugfs: create file error handling

[RFC] debugfs: create file error handling

[Stephen Hemminger]

Shouldn't Debugfs file routines should either return NULL or use ERR_PTR()
not mixed? The following patch changes the create routines to
propagate return values.

--- a/fs/debugfs/file.c	2007-05-30 10:32:32.000000000 -0700
+++ b/fs/debugfs/file.c	2007-06-18 13:38:01.000000000 -0700
@@ -83,12 +83,11 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_u8, debugfs
  * 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 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 an error occurs, an invalid pointer will be returned, use
+ * the function IS_ERR() to check. The error code can be extracted
+ * with PTR_ERR().
  */
 struct dentry *debugfs_create_u8(const char *name, mode_t mode,
 				 struct dentry *parent, u8 *value)
@@ -124,12 +123,11 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_u16, debugf
  * 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 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 an error occurs, an invalid pointer will be returned, use
+ * the function IS_ERR() to check. The error code can be extracted
+ * with PTR_ERR().
  */
 struct dentry *debugfs_create_u16(const char *name, mode_t mode,
 				  struct dentry *parent, u16 *value)
@@ -165,12 +163,11 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_u32, debugf
  * 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 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 an error occurs, an invalid pointer will be returned, use
+ * the function IS_ERR() to check. The error code can be extracted
+ * with PTR_ERR().
  */
 struct dentry *debugfs_create_u32(const char *name, mode_t mode,
 				 struct dentry *parent, u32 *value)
@@ -207,12 +204,11 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_u64, debugf
  * 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 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 an error occurs, an invalid pointer will be returned, use
+ * the function IS_ERR() to check. The error code can be extracted
+ * with PTR_ERR().
  */
 struct dentry *debugfs_create_u64(const char *name, mode_t mode,
 				 struct dentry *parent, u64 *value)
@@ -286,12 +282,11 @@ static const struct file_operations fops
  * 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 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 an error occurs, an invalid pointer will be returned, use
+ * the function IS_ERR() to check. The error code can be extracted
+ * with PTR_ERR().
  */
 struct dentry *debugfs_create_bool(const char *name, mode_t mode,
 				   struct dentry *parent, u32 *value)
@@ -330,12 +325,12 @@ static const struct file_operations fops
  * 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 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 an error occurs, an invalid pointer will be returned, use
+ * the function IS_ERR() to check. The error code can be extracted
+ * with PTR_ERR().
  */
 struct dentry *debugfs_create_blob(const char *name, mode_t mode,
 				   struct dentry *parent,
--- a/fs/debugfs/inode.c	2007-05-30 10:32:32.000000000 -0700
+++ b/fs/debugfs/inode.c	2007-06-18 13:38:26.000000000 -0700
@@ -226,14 +226,11 @@ struct dentry *debugfs_create_file(const
 	error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
 			      &debugfs_mount_count);
 	if (error)
-		goto exit;
+		goto err;
 
 	error = debugfs_create_by_name(name, mode, parent, &dentry);
-	if (error) {
-		dentry = NULL;
-		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
-		goto exit;
-	}
+	if (error)
+		goto release;
 
 	if (dentry->d_inode) {
 		if (data)
@@ -241,8 +238,12 @@ struct dentry *debugfs_create_file(const
 		if (fops)
 			dentry->d_inode->i_fop = fops;
 	}
-exit:
+
 	return dentry;
+release:
+	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+err:
+	return ERR_PTR(error);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_file);
 

Re: [RFC] debugfs: create file error handling

[Greg KH]

On Mon, Jun 18, 2007 at 01:41:32PM -0700, Stephen Hemminger wrote:
> Shouldn't Debugfs file routines should either return NULL or use ERR_PTR()
> not mixed? The following patch changes the create routines to
> propagate return values.
> 
> --- a/fs/debugfs/file.c	2007-05-30 10:32:32.000000000 -0700
> +++ b/fs/debugfs/file.c	2007-06-18 13:38:01.000000000 -0700
> @@ -83,12 +83,11 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_u8, debugfs
>   * 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 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 an error occurs, an invalid pointer will be returned, use
> + * the function IS_ERR() to check. The error code can be extracted
> + * with PTR_ERR().

No, you are forgetting the issue when debugfs is not enabled in the
kernel.

The goal is not to return -ENODEV when it's not configured in, to make
it "simpler" to handle that case.

Check the lkml archives for previous times this has come up and example
code to use to handle the error cases properly.

I agree it isn't the "simplest" way, and if you can suggest something
that is easier, and allow for the code to "easily" determine the option
isn't built in that would be great.

thanks,

greg k-h

Re: [RFC] debugfs: create file error handling

[Stephen Hemminger]

On Mon, 18 Jun 2007 13:57:27 -0700
Greg KH <greg@kroah.com> wrote:

> On Mon, Jun 18, 2007 at 01:41:32PM -0700, Stephen Hemminger wrote:
> > Shouldn't Debugfs file routines should either return NULL or use ERR_PTR()
> > not mixed? The following patch changes the create routines to
> > propagate return values.
> > 
> > --- a/fs/debugfs/file.c	2007-05-30 10:32:32.000000000 -0700
> > +++ b/fs/debugfs/file.c	2007-06-18 13:38:01.000000000 -0700
> > @@ -83,12 +83,11 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_u8, debugfs
> >   * 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 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 an error occurs, an invalid pointer will be returned, use
> > + * the function IS_ERR() to check. The error code can be extracted
> > + * with PTR_ERR().
> 
> No, you are forgetting the issue when debugfs is not enabled in the
> kernel.
> 
> The goal is not to return -ENODEV when it's not configured in, to make
> it "simpler" to handle that case.

But it makes sense to pass error values back not just: create failed
sorry!

> 
> Check the lkml archives for previous times this has come up and example
> code to use to handle the error cases properly.
> 
> I agree it isn't the "simplest" way, and if you can suggest something
> that is easier, and allow for the code to "easily" determine the option
> isn't built in that would be great.

Checking for ENODEV works fine in either case.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>