Re: [PATCH] xen/debugfs: Check debugfs initialization before using it

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Dec 09, 2013 at 05:57:22PM +0800, Ethan Zhao wrote:
> On Mon, Dec 9, 2013 at 4:55 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Dec 09, 2013 at 04:44:05PM +0800, Ethan Zhao wrote:
> >> On Mon, Dec 9, 2013 at 4:25 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Mon, Dec 09, 2013 at 09:43:16AM +0800, Ethan Zhao wrote:
> >> >> On Sun, Dec 8, 2013 at 10:01 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> >> > On Sun, Dec 08, 2013 at 07:31:02PM +0800, ethan.zhao wrote:
> >> >> >> Should check debugfs initialization with debugfs_initialized() before using it,
> >> >> >> Because if it isn't initialized, the return value of fake debugfs_create_dir() etc
> >> >> >> functions would be ERR_PTR(-ENODEV), checking with NULL will not work.
> >> >> >
> >> >> > So?  It should "just work" without this check, right?  What happens if
> >> >> > your patch isn't applied and debugfs isn't enabled?
> >> >>
> >> >> If debugfs isn't  configured, debugfs_initialized() and other
> >> >> functions are defined as following,
> >> >>
> >> >> static inline bool debugfs_initialized(void)
> >> >> {
> >> >> return false;
> >> >> }
> >> >>
> >> >> static inline struct dentry *debugfs_create_file(const char *name, umode_t mode,
> >> >> struct dentry *parent, void *data,
> >> >> const struct file_operations *fops)
> >> >> {
> >> >> return ERR_PTR(-ENODEV);
> >> >> }
> >> >>
> >> >> static inline struct dentry *debugfs_create_dir(const char *name,
> >> >> struct dentry *parent)
> >> >> {
> >> >> return ERR_PTR(-ENODEV);
> >> >> }
> >> >>
> >> >> And the checking code in xen\debugfs.c xen_init_debugfs() will not
> >> >> work, the return value is not NULL.
> >> >> d_xen_debug = debugfs_create_dir("xen", NULL);
> >> >>
> >> >> if (!d_xen_debug)
> >> >> pr_warning("Could not create 'xen' debugfs directory\n");
> >> >
> >> > Which is just fine, what is wrong with this?
> >>
> >> If we no check with debugfs_initialized(),  the above code should be
> >> if  (!d_xen_debug || IS_ERR (d_xen_debug))
> >>     pr_warning("Could not create 'xen' debugfs directory\n");
> >
> > No, you don't want to print out that message if debugfs is not enabled.
> >
> > You really don't want to print anything out, as what can a user do about
> > this?
> >
> Yep, should output a warning about "Debugfs is not configured and enabled"

No, not at all, it's not an issue you need to warn about because it was
explicitly told to be that way by the person who built that kernel.

It's not an error, please don't treat it as such.

greg k-h