Re: [PATCH] filename in tapset nfs.proc.open and nfs.proc.release

[Lukas Berk <lberk at redhat dot com>]

Hey,

David Smith <dsmith@redhat.com> writes:
> On 12/17/2014 11:36 AM, Lukas Berk wrote:
[...]
>> at argstr (rightly or wrongly) as 'just tell me everything being passed',
>> knowing I could write a more tailored script later if needed.
>
> In this case, we're not providing "everything being passed". In
> nfs.proc.open, the probe provides:

Ok, so wrongly percieved by me :)  Simply trying to point out that users
of such convience variables may be more interested having variables
easily reported to them.

> - server_ip: IP address of server
> - prot: transfer protocol
> - version: NFS version (the function is used for all NFS version)
> - filename: file name
> - flag: file flag
> - mode: file mode
>
> Only filename, flag, and mode end up in argstr.
>
> (Why was the probe written this way? I don't know, the tapset has been
> around a long time.)

Right, though I think it pertinent to echo that, we (at some point) have
decided to summarize/select variables of importance for the user, and
that it's not a direct mapping to the call itself.

> If our definition of argstr is "everything (of importance) being
> *passed*", then we'd want the filename (which is passed), not the full
> path (which isn't passed).

I'm not sure I follow why the path isn't important, given that the patch
originated due to needing the full path.  AIUI (and as mentioned below),
the parent directory *is* passed as a member variable to the file struct
($filp).  

> The kernel rarely passes around full paths internally, just a file
> pointer which contains the file name and a pointer to the dentry
> (which points us to the parent directory).

Understood, though we've recognized that the convenience variable we
provide don't always map directly to what is passed internally.  From
the perspective of the user it may not always be relevant how the kernel
passes the information.

> Also note that task_dentry_path() can fail for various reasons, while
> getting the file name from the file pointer should succeed most (if
> not all) of the time.

Again, understood, though perhaps we could fallback on just the file
pointer if task_dentry_path() fails automatically for the user?  Aim for
the best case and provide them with the information if available?

In the context of the original discussion to include the full path in
argstr; I think that providing more relevant information to the user
than less is the right way to go, especially when the full path name can
eliminate some uncertainty for the user.  Whichever way you decide to
go, I'd be happy to contribute some documentation in the synopsis
portion of the manpage.

Cheers,

Lukas