Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1212793 > unrolled thread

[PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm/backing-dev: Check return value of the debugfs_create_dir() Alexander Kuleshov <kuleshovmail@gmail.com> - 2015-08-25 10:20 +0200
    Re: [PATCH] mm/backing-dev: Check return value of the  debugfs_create_dir() Jan Kara <jack@suse.cz> - 2015-08-25 10:40 +0200
    Re: [PATCH] mm/backing-dev: Check return value of the  debugfs_create_dir() Andrew Morton <akpm@linux-foundation.org> - 2015-08-25 23:10 +0200
      Re: [PATCH] mm/backing-dev: Check return value of the  debugfs_create_dir() Jan Kara <jack@suse.cz> - 2015-08-26 11:30 +0200
        Re: [PATCH] mm/backing-dev: Check return value of the debugfs_create_dir() Alexander Kuleshov <kuleshovmail@gmail.com> - 2015-08-26 12:20 +0200

#1212793 — [PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

The debugfs_create_dir() function may fail and return error. If the
root directory not created, we can't create anything inside it. This
patch adds check for this case.

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 mm/backing-dev.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index dac5bf5..518d26a 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -117,15 +117,21 @@ static const struct file_operations bdi_debug_stats_fops = {
 
 static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
 {
-	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
-	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
-					       bdi, &bdi_debug_stats_fops);
+	if (bdi_debug_root) {
+		bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
+		if (bdi->debug_dir)
+			bdi->debug_stats = debugfs_create_file("stats", 0444,
+							bdi->debug_dir, bdi,
+							&bdi_debug_stats_fops);
+	}
 }
 
 static void bdi_debug_unregister(struct backing_dev_info *bdi)
 {
-	debugfs_remove(bdi->debug_stats);
-	debugfs_remove(bdi->debug_dir);
+	if (bdi_debug_root) {
+		debugfs_remove(bdi->debug_stats);
+		debugfs_remove(bdi->debug_dir);
+	}
 }
 #else
 static inline void bdi_debug_init(void)
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1212836 — Re: [PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

On Tue 25-08-15 13:54:23, Alexander Kuleshov wrote:
> The debugfs_create_dir() function may fail and return error. If the
> root directory not created, we can't create anything inside it. This
> patch adds check for this case.
> 
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.com>

								Honza
> ---
>  mm/backing-dev.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index dac5bf5..518d26a 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -117,15 +117,21 @@ static const struct file_operations bdi_debug_stats_fops = {
>  
>  static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
>  {
> -	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> -	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
> -					       bdi, &bdi_debug_stats_fops);
> +	if (bdi_debug_root) {
> +		bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> +		if (bdi->debug_dir)
> +			bdi->debug_stats = debugfs_create_file("stats", 0444,
> +							bdi->debug_dir, bdi,
> +							&bdi_debug_stats_fops);
> +	}
>  }
>  
>  static void bdi_debug_unregister(struct backing_dev_info *bdi)
>  {
> -	debugfs_remove(bdi->debug_stats);
> -	debugfs_remove(bdi->debug_dir);
> +	if (bdi_debug_root) {
> +		debugfs_remove(bdi->debug_stats);
> +		debugfs_remove(bdi->debug_dir);
> +	}
>  }
>  #else
>  static inline void bdi_debug_init(void)
> -- 
> 2.5.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213354 — Re: [PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

On Tue, 25 Aug 2015 13:54:23 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:

> The debugfs_create_dir() function may fail and return error. If the
> root directory not created, we can't create anything inside it. This
> patch adds check for this case.
> 
> ...
>
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -117,15 +117,21 @@ static const struct file_operations bdi_debug_stats_fops = {
>  
>  static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
>  {
> -	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> -	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
> -					       bdi, &bdi_debug_stats_fops);
> +	if (bdi_debug_root) {
> +		bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> +		if (bdi->debug_dir)
> +			bdi->debug_stats = debugfs_create_file("stats", 0444,
> +							bdi->debug_dir, bdi,
> +							&bdi_debug_stats_fops);
> +	}

If debugfs_create_dir() fails, debugfs_create_file() will go ahead and
attempt to create the debugfs file in the debugfs root directory:

: static struct dentry *start_creating(const char *name, struct dentry *parent)
: {
: ...
: 	/* If the parent is not specified, we create it in the root.
: 	 * We need the root dentry to do this, which is in the super
: 	 * block. A pointer to that is in the struct vfsmount that we
: 	 * have around.
: 	 */
: 	if (!parent)
: 		parent = debugfs_mount->mnt_root;

I'm not sure that this is very useful behaviour, and putting the files
in the wrong place is a very obscure way of informing the user that
debugfs_create_dir() failed :(


I don't think it's worth making little changes such as this - handling
debugfs failures needs a deeper rethink.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213693 — Re: [PATCH] mm/backing-dev: Check return value of the debugfs_create_dir()

On Tue 25-08-15 14:08:58, Andrew Morton wrote:
> On Tue, 25 Aug 2015 13:54:23 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:
> 
> > The debugfs_create_dir() function may fail and return error. If the
> > root directory not created, we can't create anything inside it. This
> > patch adds check for this case.
> > 
> > ...
> >
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -117,15 +117,21 @@ static const struct file_operations bdi_debug_stats_fops = {
> >  
> >  static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
> >  {
> > -	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> > -	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
> > -					       bdi, &bdi_debug_stats_fops);
> > +	if (bdi_debug_root) {
> > +		bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> > +		if (bdi->debug_dir)
> > +			bdi->debug_stats = debugfs_create_file("stats", 0444,
> > +							bdi->debug_dir, bdi,
> > +							&bdi_debug_stats_fops);
> > +	}
> 
> If debugfs_create_dir() fails, debugfs_create_file() will go ahead and
> attempt to create the debugfs file in the debugfs root directory:
> 
> : static struct dentry *start_creating(const char *name, struct dentry *parent)
> : {
> : ...
> : 	/* If the parent is not specified, we create it in the root.
> : 	 * We need the root dentry to do this, which is in the super
> : 	 * block. A pointer to that is in the struct vfsmount that we
> : 	 * have around.
> : 	 */
> : 	if (!parent)
> : 		parent = debugfs_mount->mnt_root;
> 
> I'm not sure that this is very useful behaviour, and putting the files
> in the wrong place is a very obscure way of informing the user that
> debugfs_create_dir() failed :(

But this patch actually makes sure that we don't call debugfs_create_dir()
and debugfs_create_file() with parent == NULL so this patch avoids creation
of entries in debugfs root. So IMHO it really improves the situation. And I
agree with you that falling back to debugfs root is just broken...

> I don't think it's worth making little changes such as this - handling
> debugfs failures needs a deeper rethink.

Well, handling debugfs failures like in this patch is the right way to go,
isn't it? Or what else would you imagine than checking for errors and
bailing out instead of trying to create entries in non-existent dirs?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213714

Hello Jan,

2015-08-26 15:23 GMT+06:00 Jan Kara <jack@suse.cz>:
> Well, handling debugfs failures like in this patch is the right way to go,
> isn't it? Or what else would you imagine than checking for errors and
> bailing out instead of trying to create entries in non-existent dirs?

I think Andrew talks about this thread https://lkml.org/lkml/2015/8/14/555
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web