Re: [PATCH 5/5] Document the 'info proc files' command.

[John Baldwin <jhb@FreeBSD.org>]

On 9/8/18 12:01 AM, Eli Zaretskii wrote:
>> From: John Baldwin <jhb@FreeBSD.org>
>> Date: Fri,  7 Sep 2018 17:36:59 -0700
>>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index 75436b0fc3..f5ea98ac52 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -51,6 +51,9 @@ maint set dwarf unwinders (on|off)
>>  maint show dwarf unwinders
>>    Control whether DWARF unwinders can be used.
>>  
>> +info proc files
>> +  Display a list of open files for a process.
>> +
> 
> This is OK.
> 
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index f2d1155b4d..42c077aa69 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -22236,6 +22236,14 @@ supported on @sc{gnu}/Linux and FreeBSD.
>>  Show the name of executable of the process.  This command is supported
>>  on @sc{gnu}/Linux and FreeBSD.
>>  
>> +@item info proc files
>> +@cindex info proc files
>> +Report the open file descriptors accessible in the program.  Each
> 
> Not "accessible in", "open by", right?  "Accessible" is ambiguous, it
> could mean "can potentially be accessed", and that is not what is
> shown here, AFAIU.

Ok.  I was probably trying to match the text from 'info proc mappings'
too closely.  Also, in the same vein, this should really say "process"
instead of "program" I think.  (The mappings one says program currently
but should probably also say "process" instead.)  (Oh, you already
included the "process" fix below as well.)

>> +entry displays the index and type of each descriptor.  The file name
> 
> By "index", I guess you meant the value of the descriptor, is that
> right?

Yes, not sure if there's a better way to describe that.  Looks like you
used "number" below and that is probably fine.

>> +is also listed for descriptors with an associated file name.  Network
>> +socket descriptors display the socket addresses in place of the file
>> +name.
> 
> From the example you have shown (btw, why not show it in the manual?),
> I understand that the Name field is always present, and is either a
> file or directory name, or the protocol and socket address.  If that
> is indeed correct, I suggest to reword the description:
> 
>   Show the file descriptors open by the process.  For each open file
>   descriptor, @value{GDBN} shows its number, type (file, directory,
>   character device, socket), offset, and the name of the resource open
>   on the descriptor.  The resource name can be a file name (for files,
>   directories, and devices) or a protocol followed by socket address
>   (for network connections).

This looks better to me, thanks.

> This lacks the details about "offset", which you didn't describe, and
> I couldn't guess.

Some file descriptors (for files for example) include a read/write offset
(the thing lseek() changes) used as the starting offset of read() and
write() (but not for syscalls like pread() and pwrite() that take an
explicit offset).  This value is usually referred to as the "offset" of
the file descriptor (e.g. in man pages for lseek).

>>        This command is supported on FreeBSD.
> 
> Only on FreeBSD?

My patch series only adds support for FreeBSD.  I expect someone else will
probably implement this on at least Linux.  I used this language to
match existing language of other 'info proc' subcommands in the manual
(e.g. 'info proc cmdline/cwd/exe/status' all include a sentence saying
"This command is supported on @sc{gnu}/Linux and FreeBSD.")

I don't mind adjusting it, I just think we should be consistent in the
language we use.  If we want to use something more future-proof we might
add a blanket statement to the "info proc" node to say that individual
subcommands are only supported on some systems or something to that
effect.

-- 
John Baldwin