diff mbox

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

Message ID 20180908003659.37482-6-jhb@FreeBSD.org
State New
Headers show

Commit Message

John Baldwin Sept. 8, 2018, 12:36 a.m. UTC
gdb/ChangeLog:

	* NEWS: Mention 'info proc files' command.

gdb/doc/ChangeLog:

	* gdb.texinfo (Process Information): Document "info proc files"
	command.
---
 gdb/ChangeLog       | 4 ++++
 gdb/NEWS            | 3 +++
 gdb/doc/ChangeLog   | 5 +++++
 gdb/doc/gdb.texinfo | 8 ++++++++
 4 files changed, 20 insertions(+)

Comments

Eli Zaretskii Sept. 8, 2018, 7:01 a.m. UTC | #1
> 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.

> +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?

> +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 lacks the details about "offset", which you didn't describe, and
I couldn't guess.

>        This command is supported on FreeBSD.

Only on FreeBSD?

I actually suggest to say something more future-proof, like "this
command is supported only on some systems".

Thanks.
John Baldwin Sept. 10, 2018, 6:43 p.m. UTC | #2
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 Sept. 10, 2018, 6:52 p.m. UTC | #3
On 9/8/18 12:01 AM, Eli Zaretskii wrote:
>> +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?),

Sorry, missed replying to this point.  I'm fine with adding examples to
the manual if it would be helpful.  We don't have existing examples for
other 'info proc' commands, so I was just matching the existing practice.
Eli Zaretskii Sept. 10, 2018, 7:13 p.m. UTC | #4
> Cc: gdb-patches@sourceware.org
> From: John Baldwin <jhb@FreeBSD.org>
> Date: Mon, 10 Sep 2018 11:43:16 -0700
> 
> >   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).

I'd say "file pointer offset", then.
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fb62bc55ea..720ae10ca9 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@ 
+2018-09-07  John Baldwin  <jhb@FreeBSD.org>
+
+	* NEWS: Mention 'info proc files' command.
+
 2018-09-07  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-nat.c (fbsd_nat_target::info_proc): List open file
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.
+
 * Changed commands
 
 thread apply [all | COUNT | -COUNT] [FLAG]... COMMAND
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 58e01e4f59..a9ee6834dc 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@ 
+2018-09-07  John Baldwin  <jhb@FreeBSD.org>
+
+	* gdb.texinfo (Process Information): Document "info proc files"
+	command.
+
 2018-08-29  Keith Seitz  <keiths@redhat.com>
 
 	* gdb.texinfo (Compiling and injecting code in GDB): Document
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
+entry displays the index and type of each descriptor.  The file name
+is also listed for descriptors with an associated file name.  Network
+socket descriptors display the socket addresses in place of the file
+name.  This command is supported on FreeBSD.
+
 @item info proc mappings
 @cindex memory address space mappings
 Report the memory address space ranges accessible in the program.  On