Linux Virtualization - Jul 2013 - [patch] virtio: console: fix error handling for debugfs_create_dir()

On Saturday 20 July 2013, Dan Carpenter wrote:
> On Fri, Jul 19, 2013 at 12:28:41PM +0200, Arnd Bergmann wrote:
> > On Friday 19 July 2013, Dan Carpenter wrote:
> > > debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is
disabled.
> > > Also my static checker doesn't like it when we print the
error code, but
> > > it's always just NULL.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>
> > 
> > This looks wrong.  debugfs_create_dir intentionally returns non-NULL
so
> > failing to create the directory does not trigger an error condition if
> > debugfs is disabled.
> > 
> 
> Yeah.  You're right.  But the original code is still wrong and will
> oops if debugfs is disabled.  We should set the pointer to NULL if
> we get a ERR_PTR().
> 
> I will send a v2 patch.
I don't see where that oops would happen. In the code I'm looking at,
all uses of ->debugfs_dir only ever get passed into other debugfs
functions that are stubbed out to empty inline functions.

It's not the most obvious interface design, but this all seems intentional
and correct to me.

	Arnd

On Sun, Jul 21, 2013 at 11:36:25AM +0200, Arnd Bergmann
wrote:
> On Saturday 20 July 2013, Dan Carpenter wrote:
> > On Fri, Jul 19, 2013 at 12:28:41PM +0200, Arnd Bergmann wrote:
> > > On Friday 19 July 2013, Dan Carpenter wrote:
> > > > debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is
disabled.
> > > > Also my static checker doesn't like it when we print the
error code, but
> > > > it's always just NULL.
> > > > 
> > > > Signed-off-by: Dan Carpenter <dan.carpenter at
oracle.com>
> > > 
> > > This looks wrong.  debugfs_create_dir intentionally returns
non-NULL so
> > > failing to create the directory does not trigger an error
condition if
> > > debugfs is disabled.
> > > 
> > 
> > Yeah.  You're right.  But the original code is still wrong and
will
> > oops if debugfs is disabled.  We should set the pointer to NULL if
> > we get a ERR_PTR().
> > 
> > I will send a v2 patch.
> 
> I don't see where that oops would happen. In the code I'm looking
at,
> all uses of ->debugfs_dir only ever get passed into other debugfs
> functions that are stubbed out to empty inline functions.
> 
> It's not the most obvious interface design, but this all seems
intentional
> and correct to me.
It was the best interface design I could create, making it very easy for
drivers to use and not really worry at all if debugfs was failing or
not, or if it was even present in the system or not.  That was the
design goal I had for it when I wrote it.

thanks,

greg k-h

> I don't see where that oops would happen. In the code I'm looking
at,
> all uses of ->debugfs_dir only ever get passed into other debugfs
> functions that are stubbed out to empty inline functions.
> 
> It's not the most obvious interface design, but this all seems
intentional
> and correct to me.
Ah.  Ok.  You're right.

regards,
dan carpenter