Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Mario Rugiero <mrugiero@gmail.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org,
	Adrian Hunter <adrian.hunter@intel.com>,
	Artem Bityutskiy <dedekind1@gmail.com>
Subject: Re: [PATCH] ubifs: no need to check return value of debugfs_create functions
Date: Wed, 12 Jun 2019 13:52:23 -0300
Message-ID: <CAKKQwLSm5FchbBH43G=3hJJ3LdrcOxnEn10_cjES+MZotVCpng@mail.gmail.com> (raw)
In-Reply-To: <20190612152120.GA17450@kroah.com>

I think you should still abort if the directory fails to be created.
If it's NULL, all later file creation will be attempted at root debugfs.
Not only will they pollute the filesystem, but since they are not
attached to the directory we remove on cleanup, they'll leak.
Also, since failure is no longer reported, it may be a good idea to
log that failure.

El mié., 12 jun. 2019 a las 12:21, Greg Kroah-Hartman
(<gregkh@linuxfoundation.org>) escribió:
>
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Artem Bityutskiy <dedekind1@gmail.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: linux-mtd@lists.infradead.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  fs/ubifs/debug.c | 167 ++++++++++++-----------------------------------
>  fs/ubifs/debug.h |   4 +-
>  fs/ubifs/super.c |   9 +--
>  3 files changed, 45 insertions(+), 135 deletions(-)
>
> diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
> index 3a2613038e88..57e6fcb043ca 100644
> --- a/fs/ubifs/debug.c
> +++ b/fs/ubifs/debug.c
> @@ -2812,115 +2812,69 @@ static const struct file_operations dfs_fops = {
>   * dbg_debugfs_init_fs - initialize debugfs for UBIFS instance.
>   * @c: UBIFS file-system description object
>   *
> - * This function creates all debugfs files for this instance of UBIFS. Returns
> - * zero in case of success and a negative error code in case of failure.
> + * This function creates all debugfs files for this instance of UBIFS.
>   *
>   * Note, the only reason we have not merged this function with the
>   * 'ubifs_debugging_init()' function is because it is better to initialize
>   * debugfs interfaces at the very end of the mount process, and remove them at
>   * the very beginning of the mount process.
>   */
> -int dbg_debugfs_init_fs(struct ubifs_info *c)
> +void dbg_debugfs_init_fs(struct ubifs_info *c)
>  {
>         int err, n;
>         const char *fname;
> -       struct dentry *dent;
>         struct ubifs_debug_info *d = c->dbg;
>
> -       if (!IS_ENABLED(CONFIG_DEBUG_FS))
> -               return 0;
> -
>         n = snprintf(d->dfs_dir_name, UBIFS_DFS_DIR_LEN + 1, UBIFS_DFS_DIR_NAME,
>                      c->vi.ubi_num, c->vi.vol_id);
>         if (n == UBIFS_DFS_DIR_LEN) {
>                 /* The array size is too small */
>                 fname = UBIFS_DFS_DIR_NAME;
> -               dent = ERR_PTR(-EINVAL);
> -               goto out;
> +               return;
>         }
>
>         fname = d->dfs_dir_name;
> -       dent = debugfs_create_dir(fname, dfs_rootdir);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out;
> -       d->dfs_dir = dent;
> +       d->dfs_dir = debugfs_create_dir(fname, dfs_rootdir);
>
>         fname = "dump_lprops";
> -       dent = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, c, &dfs_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       d->dfs_dump_lprops = dent;
> +       d->dfs_dump_lprops = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, c,
> +                                                &dfs_fops);
>
>         fname = "dump_budg";
> -       dent = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, c, &dfs_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       d->dfs_dump_budg = dent;
> +       d->dfs_dump_budg = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, c,
> +                                              &dfs_fops);
>
>         fname = "dump_tnc";
> -       dent = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, c, &dfs_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       d->dfs_dump_tnc = dent;
> +       d->dfs_dump_tnc = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, c,
> +                                             &dfs_fops);
>
>         fname = "chk_general";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, d->dfs_dir, c,
> -                                  &dfs_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       d->dfs_chk_gen = dent;
> +       d->dfs_chk_gen = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                            d->dfs_dir, c, &dfs_fops);
>
>         fname = "chk_index";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, d->dfs_dir, c,
> -                                  &dfs_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       d->dfs_chk_index = dent;
> +       d->dfs_chk_index = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                              d->dfs_dir, c, &dfs_fops);
>
>         fname = "chk_orphans";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, d->dfs_dir, c,
> -                                  &dfs_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       d->dfs_chk_orph = dent;
> +       d->dfs_chk_orph = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                             d->dfs_dir, c, &dfs_fops);
>
>         fname = "chk_lprops";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, d->dfs_dir, c,
> -                                  &dfs_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       d->dfs_chk_lprops = dent;
> +       d->dfs_chk_lprops = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                               d->dfs_dir, c, &dfs_fops);
>
>         fname = "chk_fs";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, d->dfs_dir, c,
> -                                  &dfs_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       d->dfs_chk_fs = dent;
> +       d->dfs_chk_fs = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                           d->dfs_dir, c, &dfs_fops);
>
>         fname = "tst_recovery";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, d->dfs_dir, c,
> -                                  &dfs_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       d->dfs_tst_rcvry = dent;
> +       d->dfs_tst_rcvry = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                              d->dfs_dir, c, &dfs_fops);
>
>         fname = "ro_error";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, d->dfs_dir, c,
> -                                  &dfs_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       d->dfs_ro_error = dent;
> -
> -       return 0;
> -
> -out_remove:
> -       debugfs_remove_recursive(d->dfs_dir);
> -out:
> -       err = dent ? PTR_ERR(dent) : -ENODEV;
> -       ubifs_err(c, "cannot create \"%s\" debugfs file or directory, error %d\n",
> -                 fname, err);
> -       return err;
> +       d->dfs_ro_error = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                             d->dfs_dir, c, &dfs_fops);
>  }
>
>  /**
> @@ -2929,8 +2883,7 @@ int dbg_debugfs_init_fs(struct ubifs_info *c)
>   */
>  void dbg_debugfs_exit_fs(struct ubifs_info *c)
>  {
> -       if (IS_ENABLED(CONFIG_DEBUG_FS))
> -               debugfs_remove_recursive(c->dbg->dfs_dir);
> +       debugfs_remove_recursive(c->dbg->dfs_dir);
>  }
>
>  struct ubifs_global_debug_info ubifs_dbg;
> @@ -3006,75 +2959,38 @@ static const struct file_operations dfs_global_fops = {
>   *
>   * UBIFS uses debugfs file-system to expose various debugging knobs to
>   * user-space. This function creates "ubifs" directory in the debugfs
> - * file-system. Returns zero in case of success and a negative error code in
> - * case of failure.
> + * file-system.
>   */
> -int dbg_debugfs_init(void)
> +void dbg_debugfs_init(void)
>  {
> -       int err;
>         const char *fname;
> -       struct dentry *dent;
> -
> -       if (!IS_ENABLED(CONFIG_DEBUG_FS))
> -               return 0;
>
>         fname = "ubifs";
> -       dent = debugfs_create_dir(fname, NULL);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out;
> -       dfs_rootdir = dent;
> +       dfs_rootdir = debugfs_create_dir(fname, NULL);
>
>         fname = "chk_general";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, dfs_rootdir, NULL,
> -                                  &dfs_global_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       dfs_chk_gen = dent;
> +       dfs_chk_gen = debugfs_create_file(fname, S_IRUSR | S_IWUSR, dfs_rootdir,
> +                                         NULL, &dfs_global_fops);
>
>         fname = "chk_index";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, dfs_rootdir, NULL,
> -                                  &dfs_global_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       dfs_chk_index = dent;
> +       dfs_chk_index = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                           dfs_rootdir, NULL, &dfs_global_fops);
>
>         fname = "chk_orphans";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, dfs_rootdir, NULL,
> -                                  &dfs_global_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       dfs_chk_orph = dent;
> +       dfs_chk_orph = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                          dfs_rootdir, NULL, &dfs_global_fops);
>
>         fname = "chk_lprops";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, dfs_rootdir, NULL,
> -                                  &dfs_global_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       dfs_chk_lprops = dent;
> +       dfs_chk_lprops = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                            dfs_rootdir, NULL, &dfs_global_fops);
>
>         fname = "chk_fs";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, dfs_rootdir, NULL,
> -                                  &dfs_global_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       dfs_chk_fs = dent;
> +       dfs_chk_fs = debugfs_create_file(fname, S_IRUSR | S_IWUSR, dfs_rootdir,
> +                                        NULL, &dfs_global_fops);
>
>         fname = "tst_recovery";
> -       dent = debugfs_create_file(fname, S_IRUSR | S_IWUSR, dfs_rootdir, NULL,
> -                                  &dfs_global_fops);
> -       if (IS_ERR_OR_NULL(dent))
> -               goto out_remove;
> -       dfs_tst_rcvry = dent;
> -
> -       return 0;
> -
> -out_remove:
> -       debugfs_remove_recursive(dfs_rootdir);
> -out:
> -       err = dent ? PTR_ERR(dent) : -ENODEV;
> -       pr_err("UBIFS error (pid %d): cannot create \"%s\" debugfs file or directory, error %d\n",
> -              current->pid, fname, err);
> -       return err;
> +       dfs_tst_rcvry = debugfs_create_file(fname, S_IRUSR | S_IWUSR,
> +                                           dfs_rootdir, NULL, &dfs_global_fops);
>  }
>
>  /**
> @@ -3082,8 +2998,7 @@ int dbg_debugfs_init(void)
>   */
>  void dbg_debugfs_exit(void)
>  {
> -       if (IS_ENABLED(CONFIG_DEBUG_FS))
> -               debugfs_remove_recursive(dfs_rootdir);
> +       debugfs_remove_recursive(dfs_rootdir);
>  }
>
>  void ubifs_assert_failed(struct ubifs_info *c, const char *expr,
> diff --git a/fs/ubifs/debug.h b/fs/ubifs/debug.h
> index 64c6977c189b..c6c8f1b62af5 100644
> --- a/fs/ubifs/debug.h
> +++ b/fs/ubifs/debug.h
> @@ -309,9 +309,9 @@ int dbg_leb_unmap(struct ubifs_info *c, int lnum);
>  int dbg_leb_map(struct ubifs_info *c, int lnum);
>
>  /* Debugfs-related stuff */
> -int dbg_debugfs_init(void);
> +void dbg_debugfs_init(void);
>  void dbg_debugfs_exit(void);
> -int dbg_debugfs_init_fs(struct ubifs_info *c);
> +void dbg_debugfs_init_fs(struct ubifs_info *c);
>  void dbg_debugfs_exit_fs(struct ubifs_info *c);
>
>  #endif /* !__UBIFS_DEBUG_H__ */
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 04b8ecfd3470..2429906bd681 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1477,9 +1477,7 @@ static int mount_ubifs(struct ubifs_info *c)
>         if (err)
>                 goto out_infos;
>
> -       err = dbg_debugfs_init_fs(c);
> -       if (err)
> -               goto out_infos;
> +       dbg_debugfs_init_fs(c);
>
>         c->mounting = 0;
>
> @@ -2364,9 +2362,7 @@ static int __init ubifs_init(void)
>         if (err)
>                 goto out_shrinker;
>
> -       err = dbg_debugfs_init();
> -       if (err)
> -               goto out_compr;
> +       dbg_debugfs_init();
>
>         err = register_filesystem(&ubifs_fs_type);
>         if (err) {
> @@ -2378,7 +2374,6 @@ static int __init ubifs_init(void)
>
>  out_dbg:
>         dbg_debugfs_exit();
> -out_compr:
>         ubifs_compressors_exit();
>  out_shrinker:
>         unregister_shrinker(&ubifs_shrinker_info);
> --
> 2.22.0
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-12 15:21 Greg Kroah-Hartman
2019-06-12 16:52 ` Mario Rugiero [this message]
2019-06-12 17:00   ` Greg Kroah-Hartman
2019-06-12 18:53 ` Richard Weinberger
2019-06-13  5:47   ` Greg Kroah-Hartman

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKKQwLSm5FchbBH43G=3hJJ3LdrcOxnEn10_cjES+MZotVCpng@mail.gmail.com' \
    --to=mrugiero@gmail.com \
    --cc=adrian.hunter@intel.com \
    --cc=dedekind1@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=richard@nod.at \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org linux-mtd@archiver.kernel.org
	public-inbox-index linux-mtd


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/ public-inbox