Do not clear the value of st_dev in File I/O's stat()

  Do not clear the value of st_dev in the fileio stat structure sent to the
target. It prevents from being able to check the file type on the target.
Note that the fileio function fstat `remote_fileio_func_fstat()` doesn't clear
this field.

2018-05-16  Julio Guerra  <julio@farjump.io>

	* remote-fileio.c: do not clear the value of st_dev in File I/O's stat().

Signed-off-by: Julio Guerra <julio@farjump.io>
---
 gdb/ChangeLog       | 4 ++++
 gdb/remote-fileio.c | 1 -
 2 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.17.0
  

[Hi Corinna, adding you in case you still happen to remember
any of this and have any input.  If not, that's fine.]

On 05/17/2018 09:28 AM, Julio Guerra wrote:
> Do not clear the value of st_dev in the fileio stat structure sent to the
> target. It prevents from being able to check the file type on the target.
> Note that the fileio function fstat `remote_fileio_func_fstat()` doesn't clear
> this field.

Curious.  This looked like was written in a way like you'd write if you
had a reason to so I did some archaeology to try to find what it was, see
if the reason is still valid.

I found the original File I/O protocol proposal here:

  https://www.sourceware.org/ml/gdb/2002-11/msg00107.html

and in there, in the intro, it said:

~~~~~
  The protocol is only used for files on the host file system and
  for I/O on the console.  Character or block special devices, pipes,
  named pipes or sockets or any other communication method on the host
  system are not supported by this protocol.
~~~~~

and further below, in "B.3 struct stat", it says:

~~~~~
  The values of several fields have a restricted meaning and/or
  range of values.

    st_dev:	0	file
		1	console
~~~~~

And the current manual also says:

 @item st_dev
 A value of 0 represents a file, 1 the console.

And it looks like early on, in:

 https://sourceware.org/ml/gdb-patches/2003-03/msg00221.html

we had:

 +static void
 +remote_fileio_to_fio_stat (struct stat *st, struct fio_stat *fst)
 +{
 +  /* `st_dev' is set in the calling function */
 +  remote_fileio_to_fio_uint ((long) st->st_ino, fst->fst_ino);

Note the comment.  

I can't find the comment in the current sources, but it was there
up until:

 commit 791c00567a7ccbae3d71e3b63ac43c0b555079dc
 Author:     Gary Benson <gbenson@redhat.com>
 AuthorDate: Wed Mar 11 17:53:57 2015 +0000

    Move remote_fileio_to_fio_stat to gdb/common


and note that remote_fileio_func_fstat still does:

  if (fd == FIO_FD_CONSOLE_IN || fd == FIO_FD_CONSOLE_OUT)
    {
      host_to_fileio_uint (1, fst.fst_dev);
                          ^^^

"fstat" was only added later on, along the work that Gary did
around that commit above, for this series:

 https://sourceware.org/ml/gdb-patches/2015-03/msg00286.html

... which introduced "fstat" in the protocol, and in that case,
the 0/1 st_dev issue was either missed or ignored, so
fstat is not following the documented values.

Now I'm not sure what to do.  If we let the real st_dev
through, what shall we fill it in, in the "console" files
case?

> 
> 2018-05-16  Julio Guerra  <julio@farjump.io>
> 
> 	* remote-fileio.c: do not clear the value of st_dev in File I/O's stat().

Nit, you'd write:

gdb/ChangeLog:
2018-05-16  Julio Guerra  <julio@farjump.io>

	* remote-fileio.c (remote_fileio_func_stat): Do not clear the
	value of st_dev.

Thanks,
Pedro Alves
  

Le 17/05/2018 à 16:36, Pedro Alves a écrit :
> [Hi Corinna, adding you in case you still happen to remember

> any of this and have any input.  If not, that's fine.]

>

> On 05/17/2018 09:28 AM, Julio Guerra wrote:

>> Do not clear the value of st_dev in the fileio stat structure sent to the

>> target. It prevents from being able to check the file type on the target.

>> Note that the fileio function fstat `remote_fileio_func_fstat()` doesn't clear

>> this field.

> Curious.  This looked like was written in a way like you'd write if you

> had a reason to so I did some archaeology to try to find what it was, see

> if the reason is still valid.

>

> I found the original File I/O protocol proposal here:

>

>   https://www.sourceware.org/ml/gdb/2002-11/msg00107.html

>

> and in there, in the intro, it said:

>

> ~~~~~

>   The protocol is only used for files on the host file system and

>   for I/O on the console.  Character or block special devices, pipes,

>   named pipes or sockets or any other communication method on the host

>   system are not supported by this protocol.

> ~~~~~

>

> and further below, in "B.3 struct stat", it says:

>

> ~~~~~

>   The values of several fields have a restricted meaning and/or

>   range of values.

>

>     st_dev:	0	file

> 		1	console

> ~~~~~

>

> And the current manual also says:

>

>  @item st_dev

>  A value of 0 represents a file, 1 the console.

>

> And it looks like early on, in:

>

>  https://sourceware.org/ml/gdb-patches/2003-03/msg00221.html

>

> we had:

>

>  +static void

>  +remote_fileio_to_fio_stat (struct stat *st, struct fio_stat *fst)

>  +{

>  +  /* `st_dev' is set in the calling function */

>  +  remote_fileio_to_fio_uint ((long) st->st_ino, fst->fst_ino);

>

> Note the comment.  

>

> I can't find the comment in the current sources, but it was there

> up until:

>

>  commit 791c00567a7ccbae3d71e3b63ac43c0b555079dc

>  Author:     Gary Benson <gbenson@redhat.com>

>  AuthorDate: Wed Mar 11 17:53:57 2015 +0000

>

>     Move remote_fileio_to_fio_stat to gdb/common

>

>

> and note that remote_fileio_func_fstat still does:

>

>   if (fd == FIO_FD_CONSOLE_IN || fd == FIO_FD_CONSOLE_OUT)

>     {

>       host_to_fileio_uint (1, fst.fst_dev);

>                           ^^^

>

> "fstat" was only added later on, along the work that Gary did

> around that commit above, for this series:

>

>  https://sourceware.org/ml/gdb-patches/2015-03/msg00286.html

>

> ... which introduced "fstat" in the protocol, and in that case,

> the 0/1 st_dev issue was either missed or ignored, so

> fstat is not following the documented values.

>

> Now I'm not sure what to do.  If we let the real st_dev

> through, what shall we fill it in, in the "console" files

> case?

>

Well, this is a little bit like the other patch I submitted (allowing to
open non regular files): I think that File IO values should just pass
through GDB. So running stat() or fstat() on the console (stdin/stdout)
should simply return the same as host's, ie. equivalent to fstat(0 or 1).

>> 2018-05-16  Julio Guerra  <julio@farjump.io>

>>

>> 	* remote-fileio.c: do not clear the value of st_dev in File I/O's stat().

> Nit, you'd write:

>

> gdb/ChangeLog:

> 2018-05-16  Julio Guerra  <julio@farjump.io>

>

> 	* remote-fileio.c (remote_fileio_func_stat): Do not clear the

> 	value of st_dev.

Sorry, I'm not familiar with this yet.

-- 
Julio Guerra
  

Hi Pedro,

On May 17 15:36, Pedro Alves wrote:
> [Hi Corinna, adding you in case you still happen to remember
> any of this and have any input.  If not, that's fine.]

Sorry for the late reply, just back from vacation.  I remember this
stuff patially only, but in terms of st_dev, I recall some discussion.

> On 05/17/2018 09:28 AM, Julio Guerra wrote:
> > Do not clear the value of st_dev in the fileio stat structure sent to the
> > target. It prevents from being able to check the file type on the target.
> > Note that the fileio function fstat `remote_fileio_func_fstat()` doesn't clear
> > this field.
> 
> Curious.  This looked like was written in a way like you'd write if you
> had a reason to so I did some archaeology to try to find what it was, see
> if the reason is still valid.
> 
> I found the original File I/O protocol proposal here:
> 
>   https://www.sourceware.org/ml/gdb/2002-11/msg00107.html
> 
> and in there, in the intro, it said:
> 
> ~~~~~
>   The protocol is only used for files on the host file system and
>   for I/O on the console.  Character or block special devices, pipes,
>   named pipes or sockets or any other communication method on the host
>   system are not supported by this protocol.
> ~~~~~
> 
> and further below, in "B.3 struct stat", it says:
> 
> ~~~~~
>   The values of several fields have a restricted meaning and/or
>   range of values.
> 
>     st_dev:	0	file
> 		1	console
> ~~~~~
> 
> And the current manual also says:
> 
>  @item st_dev
>  A value of 0 represents a file, 1 the console.
> [...]

The idea was that st_dev values on one system don't make sense on
another system.  Also, st_dev values for files reflect the underlying
partition on the inferior system, which doesn't mean anything to the GDB
host system.

I don't know how important backward compat is here, but there may
be code out there which still relies on the simple file vs. console
st_dev from stat().  Version check?

Other than that, if you think that this is an outdated approach, just
make sure to change the docs as well.


HTH,
Corinna
  

> The idea was that st_dev values on one system don't make sense on
> another system.  Also, st_dev values for files reflect the underlying
> partition on the inferior system, which doesn't mean anything to the GDB
> host system.
>
> I don't know how important backward compat is here, but there may
> be code out there which still relies on the simple file vs. console
> st_dev from stat().  Version check?
>
> Other than that, if you think that this is an outdated approach, just
> make sure to change the docs as well.

I think the approach is still valid, otherwise it would be like if the
target program should know the host. Which reminds me that with
remote_fileio_fstat(), I have the case where my host st_dev has 64 bits,
which doesn't fit into fileio's 32-bit fst_dev.

I think I should resubmit this patch along with my other patch allowing
to "open non regular files" so that I add common device ids (link, fifo,
etc.) to the list of returned fst_dev, and I modify
remote_fileio_fstat() so that it applies the same values to fst_dev.
  

On 05/28/2018 01:23 PM, Julio Guerra wrote:

> I think the approach is still valid, otherwise it would be like if the
> target program should know the host. Which reminds me that with
> remote_fileio_fstat(), I have the case where my host st_dev has 64 bits,
> which doesn't fit into fileio's 32-bit fst_dev.
> 
> I think I should resubmit this patch along with my other patch allowing
> to "open non regular files" so that I add common device ids (link, fifo,
> etc.) to the list of returned fst_dev, and I modify
> remote_fileio_fstat() so that it applies the same values to fst_dev.

Great, that sounds like the right direction.

Thanks.  And thanks Corinna.

Thanks,
Pedro Alves