Re: [PATCH] xfs: fix file_path handling in tracepoints

[Christoph Hellwig <hch@infradead.org>]

On Thu, Jul 11, 2024 at 09:17:54PM -0400, Steven Rostedt wrote:
> That "f->f_path.dentry" is a dereference of the passed in pointer. If we
> did that in the TP_printk(), then it would dereference that file pointer
> saved by the trace. This would happen at some time later from when the file
> pointer was saved. That is, it will dereference the pointer when the user
> reads the trace, not when the trace occurred. This could be seconds,
> minutes, hours, days even months later! So %pD would not work there.

Indeed.  I'm so used to these useful format strings that I keep
forgetting about them doing non-trivial things.

Which also brings up that it would be good if we had some kind of static
checker that detects usage of these magic %p extensions in the trace
macros and warns about them.

> 		__dynamic_array(char, pathname, snprintf(NULL, 0, "%pD", xf->file) + 1);
> 
> // This will allocated the space needed for the string
> 

> 		sprintf(__get_dynamic_array(pathname), "%pD", xf->file);
> 
> // and the above will copy it directly to that location.
> // It assumes the value of the first snprintf() will be the same as the second.
> 

> 		  (char *)__get_dynamic_array(pathname),
> 
> // for accessing the string, although yes, __get_str(pathname) would work,
> // but that's more by luck than design.

That sounds pretty cool, but except for the dynamic sizing doesn't
really buy us much over the version Darrick proposed, right?

> Looking at this file, I noticed that you have some open coded __string_len()
> fields. Why not just use that? In fact, I think I even found a bug:
> 
> There's a:
> 		memcpy(__get_str(name), name, name->len);
> 
> Where I think it should have been:
> 
> 		memcpy(__get_str(name), name->name, name->len);
> 
> Hmm, I should make sure that __string() and __string_len() are passed in
> strings. As this is a common bug.
> 
> I can make this a formal patch if you like. Although, I haven't even tried
> compile testing it ;-)

Without having compiled it either, this looks sensible to me.  As XFS
was one of the earliest adopters of event tracing I suspect these
predate the string helpers.