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

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

[Ethan Zhao]

From: "Ethan Zhao" <ethan.kernel@gmail.com>

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.

V3: Add warning message about debugfs not configured or disabled. 

Signed-off-by: Ethan Zhao <ethan.kernel@gmail.com>
---
 arch/x86/xen/debugfs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/xen/debugfs.c b/arch/x86/xen/debugfs.c
index c8377fb..033a5dd 100644
--- a/arch/x86/xen/debugfs.c
+++ b/arch/x86/xen/debugfs.c
@@ -9,12 +9,19 @@ static struct dentry *d_xen_debug;
 
 struct dentry * __init xen_init_debugfs(void)
 {
+	if (!debugfs_initialized()) {
+		d_xen_debug = NULL;
+		pr_warning("debugfs is not configured or enabled\n");
+		goto nodebugfs;
+	}
+
 	if (!d_xen_debug) {
 		d_xen_debug = debugfs_create_dir("xen", NULL);
 
 		if (!d_xen_debug)
 			pr_warning("Could not create 'xen' debugfs directory\n");
 	}
+nodebugfs:
 
 	return d_xen_debug;
 }
-- 
1.8.3.4 (Apple Git-47)

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

[Greg KH]

On Mon, Dec 09, 2013 at 06:04:03PM +0800, Ethan Zhao wrote:
> From: "Ethan Zhao" <ethan.kernel@gmail.com>
> 
> 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.
> 
> V3: Add warning message about debugfs not configured or disabled. 
> 
> Signed-off-by: Ethan Zhao <ethan.kernel@gmail.com>
> ---
>  arch/x86/xen/debugfs.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/xen/debugfs.c b/arch/x86/xen/debugfs.c
> index c8377fb..033a5dd 100644
> --- a/arch/x86/xen/debugfs.c
> +++ b/arch/x86/xen/debugfs.c
> @@ -9,12 +9,19 @@ static struct dentry *d_xen_debug;
>  
>  struct dentry * __init xen_init_debugfs(void)
>  {
> +	if (!debugfs_initialized()) {
> +		d_xen_debug = NULL;
> +		pr_warning("debugfs is not configured or enabled\n");

No, again, don't say anything, this is useless, just leave the code
as-is, it isn't causing any problems, is it?

gre gk-h

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

[Ethan Zhao]

On Tue, Dec 10, 2013 at 4:19 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Dec 10, 2013 at 04:03:41PM +0800, Ethan Zhao wrote:
>> On Tue, Dec 10, 2013 at 2:40 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Dec 09, 2013 at 09:42:23PM +0800, Ethan Zhao wrote:
>> >> Greg,
>> >>     I am the man who built a Xen dom0, but couldn't see debugfs
>> >> directory and files as expected. there is no warning or tip for me to
>> >> enable the CONFIG_DEBUG_FS=y in .config , it cost me minutes to figure
>> >> out what's
>> >> the matter. and I know should check defugfs config and initialization as
>> >> zswap_debugfs, tracer_debugfs ,rproc_debugfs did. Is it useless ? if
>> >> it could save me just 1 minute next time ?
>> >
>> > So you would want a "warning" showing up for every single part of the
>> > kernel that uses debugfs for when it isn't enabled?  That doesn't make
>> > too much sense now, does it?
>>
>> No, It is nice and like sun light when someone is struggling with the
>> bugs in darkness,
>> if some tips or warning output to them.
>>
>> You have forgotten the initial stage you met :)
>
> So, you really want to see 20+ KERNEL WARNINGS in your system when you
> boot without CONFIG_DEBUGFS enabled?  No, that's not ok at all, sorry,
> that is not going to happen.

You got the right reason to make me give up !

Thanks,
Ethan
>
> Running a kernel without debugfs is a valid state, you are treating it
> as an error, which isn't ok.
>
> greg k-h

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

[Greg KH]

On Tue, Dec 10, 2013 at 04:03:41PM +0800, Ethan Zhao wrote:
> On Tue, Dec 10, 2013 at 2:40 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Dec 09, 2013 at 09:42:23PM +0800, Ethan Zhao wrote:
> >> Greg,
> >>     I am the man who built a Xen dom0, but couldn't see debugfs
> >> directory and files as expected. there is no warning or tip for me to
> >> enable the CONFIG_DEBUG_FS=y in .config , it cost me minutes to figure
> >> out what's
> >> the matter. and I know should check defugfs config and initialization as
> >> zswap_debugfs, tracer_debugfs ,rproc_debugfs did. Is it useless ? if
> >> it could save me just 1 minute next time ?
> >
> > So you would want a "warning" showing up for every single part of the
> > kernel that uses debugfs for when it isn't enabled?  That doesn't make
> > too much sense now, does it?
> 
> No, It is nice and like sun light when someone is struggling with the
> bugs in darkness,
> if some tips or warning output to them.
> 
> You have forgotten the initial stage you met :)

So, you really want to see 20+ KERNEL WARNINGS in your system when you
boot without CONFIG_DEBUGFS enabled?  No, that's not ok at all, sorry,
that is not going to happen.

Running a kernel without debugfs is a valid state, you are treating it
as an error, which isn't ok.

greg k-h

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

[Ethan Zhao]

On Tue, Dec 10, 2013 at 2:40 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Dec 09, 2013 at 09:42:23PM +0800, Ethan Zhao wrote:
>> Greg,
>>     I am the man who built a Xen dom0, but couldn't see debugfs
>> directory and files as expected. there is no warning or tip for me to
>> enable the CONFIG_DEBUG_FS=y in .config , it cost me minutes to figure
>> out what's
>> the matter. and I know should check defugfs config and initialization as
>> zswap_debugfs, tracer_debugfs ,rproc_debugfs did. Is it useless ? if
>> it could save me just 1 minute next time ?
>
> So you would want a "warning" showing up for every single part of the
> kernel that uses debugfs for when it isn't enabled?  That doesn't make
> too much sense now, does it?

No, It is nice and like sun light when someone is struggling with the
bugs in darkness,
if some tips or warning output to them.

You have forgotten the initial stage you met :)

>
> If you need/want debugfs then enable it in the kernel build, it's not
> that hard, right?

of coz, not hard. and It not so hard to merge it too.

Thanks,
Ethan
>
> greg k-h

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

[Greg KH]

On Mon, Dec 09, 2013 at 09:42:23PM +0800, Ethan Zhao wrote:
> Greg,
>     I am the man who built a Xen dom0, but couldn't see debugfs
> directory and files as expected. there is no warning or tip for me to
> enable the CONFIG_DEBUG_FS=y in .config , it cost me minutes to figure
> out what's
> the matter. and I know should check defugfs config and initialization as
> zswap_debugfs, tracer_debugfs ,rproc_debugfs did. Is it useless ? if
> it could save me just 1 minute next time ?

So you would want a "warning" showing up for every single part of the
kernel that uses debugfs for when it isn't enabled?  That doesn't make
too much sense now, does it?

If you need/want debugfs then enable it in the kernel build, it's not
that hard, right?

greg k-h

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

[Ethan Zhao]

Greg,
    I am the man who built a Xen dom0, but couldn't see debugfs
directory and files as expected. there is no warning or tip for me to
enable the CONFIG_DEBUG_FS=y in .config , it cost me minutes to figure
out what's
the matter. and I know should check defugfs config and initialization as
zswap_debugfs, tracer_debugfs ,rproc_debugfs did. Is it useless ? if
it could save me just 1 minute next time ?

Thanks,
Ethan

On Mon, Dec 9, 2013 at 7:17 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 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

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

[Greg KH]

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

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

[Ethan Zhao]

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"

> I still think the original code is correct, have you tried it with
> debugfs disabled?

If debugfs is not configured, no "Could not create 'xen' debugfs
directory" warning.
Ok, send V3 to imporve it.

>
> greg k-h

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

[Greg KH]

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?

I still think the original code is correct, have you tried it with
debugfs disabled?

greg k-h

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

[Ethan Zhao]

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");

>
>> >
>> > greg k-h
>> >
>> >> Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>
>> >
>> > Please put your "real" name here, not one with a '.' in it.
>>
>> The real name used in company is ethan.zhao@oracle.com,  but I
>> couldn't send and receive mails of
>> community with that mailbox for some reason you may know.
>
> I mean the "ethan.zhao" part, it should be "Ethan Zhao" :)

You got the my real name, thanks. :>

Ethan
>
> thanks,
>
> greg k-h

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

[Greg KH]

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?

> >
> > greg k-h
> >
> >> Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>
> >
> > Please put your "real" name here, not one with a '.' in it.
> 
> The real name used in company is ethan.zhao@oracle.com,  but I
> couldn't send and receive mails of
> community with that mailbox for some reason you may know.

I mean the "ethan.zhao" part, it should be "Ethan Zhao" :)

thanks,

greg k-h

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

[Ethan Zhao]

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");
>
> greg k-h
>
>> Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>
>
> Please put your "real" name here, not one with a '.' in it.

The real name used in company is ethan.zhao@oracle.com,  but I
couldn't send and receive mails of
community with that mailbox for some reason you may know.

Thanks,
Ethan

>
> thanks,
>
> greg k-h

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

[Greg KH]

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?

greg k-h

> Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>

Please put your "real" name here, not one with a '.' in it.

thanks,

greg k-h

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

[ethan.zhao]

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.

Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>
---
 arch/x86/xen/debugfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/xen/debugfs.c b/arch/x86/xen/debugfs.c
index c8377fb..85c0e0e 100644
--- a/arch/x86/xen/debugfs.c
+++ b/arch/x86/xen/debugfs.c
@@ -9,12 +9,18 @@ static struct dentry *d_xen_debug;
 
 struct dentry * __init xen_init_debugfs(void)
 {
+	if (!debugfs_initialized()) {
+		d_xen_debug = NULL;
+		goto nodebugfs;
+	}
+
 	if (!d_xen_debug) {
 		d_xen_debug = debugfs_create_dir("xen", NULL);
 
 		if (!d_xen_debug)
 			pr_warning("Could not create 'xen' debugfs directory\n");
 	}
+nodebugfs:
 
 	return d_xen_debug;
 }
-- 
1.8.3.4 (Apple Git-47)