Re: [PATCH RESEND] tracing: Make tracing work when debugfs is not compiled or initialized.

[Steven Rostedt]

On Thu, 5 Nov 2015 13:23:01 +0800
Jiaxing Wang <hello.wjx@xxxxxxxxx> wrote:

> > > -	/*
> > > -	 * As there may still be users that expect the tracing
> > > -	 * files to exist in debugfs/tracing, we must automount
> > > -	 * the tracefs file system there, so older tools still
> > > -	 * work with the newer kerenl.
> > > -	 */
> > > -	tr->dir = debugfs_create_automount("tracing", NULL,
> > > -					   trace_automount, NULL);
> > > -	if (!tr->dir) {
> > > -		pr_warn_once("Could not create debugfs directory 'tracing'\n");
> > > -		return ERR_PTR(-ENOMEM);
> > > +	if (debugfs_initialized()) {
> > > +		/*
> > > +		 * As there may still be users that expect the tracing
> > > +		 * files to exist in debugfs/tracing, we must automount
> > > +		 * the tracefs file system there, so older tools still
> > > +		 * work with the newer kerenl.
> > > +		 */
> > > +		traced = debugfs_create_automount("tracing", NULL,
> > > +						   trace_automount, NULL);
> > > +		if (!traced)
> > > +			pr_warn_once("Could not create debugfs directory 'tracing'\n");
> > 
> > This should return a warning, and please keep the tr->dir instead of
> > this new traced variable.
> Do you mean return ERR_PTR(-ENOMEM); when debugfs_create_automount()
> return NULL?

Right.

> As long as tracefs is initialized, we can make tracing_init_dentry() return
> NULL even if the debugfs automount point is not created(), and tracefs can
> still be populated. If tracing_init_dentry() returns error in this case,
> the caller of tracing_init_dentry() will not populate tracefs.

But this is still a failure. tracing_init_dentry() now only mounts
tracefs on the debugfs/tracing directory. If it fails to do that when
debugfs is available, then it should fail, as it would break backward
compatibility with other tools.

If debugfs is not configured in, then it should set tr->dir to
whatever (ENOMEM is fine), and return NULL.


> > 
> > >  	}
> > >  
> > > +	tr->dir = TRACE_TOP_DIR_ENTRY;
> > > +
> > 
> > Also, no need to add this, because if debugfs is not initialize, then
> > tr->dir would be ERR_PTR(-ENODEV), which still works as tr->dir is not
> > NULL.
> If we accept debugfs_create_automount() return NULL, tr->dir can still
> be NULL if we do tr->dir = debugfs_create_automount().

What's wrong with that? This function is only to automount debugfs now.

Also, I think the first check should be:

	if (WARN_ON(!tracefs_initialized()) ||
	    (IS_ENABLED(CONFIG_DEBUGFS) &&
	     WARN_ON(!debugfs_initialized()))
		return ERR_PTR(-ENODEV);

Then we don't need the if (debugfs_initialized()) conditional.

-- Steve


> > 
> > -- Steve
> > 
> > >  	return NULL;
> > >  }
> > >  
> > 

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