* [PATCH] Do not clear the value of st_dev in File I/O's stat()
[not found] <20180517082631.26855-1-julio@farjump.io>
@ 2018-05-17 10:32 ` Julio Guerra
2018-05-17 15:31 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Julio Guerra @ 2018-05-17 10:32 UTC (permalink / raw)
To: gdb-patches; +Cc: Pedro Alves, Julio Guerra
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(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7217be67b6..34e7995e5a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+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().
+
2018-05-16 Julio Guerra <julio@farjump.io>
* remote-fileio.c: allow using File I/O function open() with special
diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index fa3cb15033..e855c682a0 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -870,7 +870,6 @@ remote_fileio_func_stat (char *buf)
if (statptr)
{
host_to_fileio_stat (&st, &fst);
- host_to_fileio_uint (0, fst.fst_dev);
errno = target_write_memory (statptr, (gdb_byte *) &fst, sizeof fst);
if (errno != 0)
--
2.17.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Do not clear the value of st_dev in File I/O's stat()
2018-05-17 10:32 ` [PATCH] Do not clear the value of st_dev in File I/O's stat() Julio Guerra
@ 2018-05-17 15:31 ` Pedro Alves
[not found] ` <496efebf-0f96-4586-de39-0a1857994f04@farjump.io>
2018-05-28 10:50 ` Corinna Vinschen
0 siblings, 2 replies; 6+ messages in thread
From: Pedro Alves @ 2018-05-17 15:31 UTC (permalink / raw)
To: Julio Guerra, gdb-patches; +Cc: Corinna Vinschen
[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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Do not clear the value of st_dev in File I/O's stat()
[not found] ` <496efebf-0f96-4586-de39-0a1857994f04@farjump.io>
@ 2018-05-18 18:29 ` Julio Guerra
0 siblings, 0 replies; 6+ messages in thread
From: Julio Guerra @ 2018-05-18 18:29 UTC (permalink / raw)
To: Pedro Alves, gdb-patches; +Cc: Corinna Vinschen
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Do not clear the value of st_dev in File I/O's stat()
2018-05-17 15:31 ` Pedro Alves
[not found] ` <496efebf-0f96-4586-de39-0a1857994f04@farjump.io>
@ 2018-05-28 10:50 ` Corinna Vinschen
[not found] ` <6f5980ae-247c-1991-ba3c-884fe04a94c5@farjump.io>
1 sibling, 1 reply; 6+ messages in thread
From: Corinna Vinschen @ 2018-05-28 10:50 UTC (permalink / raw)
To: Pedro Alves; +Cc: Julio Guerra, gdb-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Do not clear the value of st_dev in File I/O's stat()
[not found] ` <6f5980ae-247c-1991-ba3c-884fe04a94c5@farjump.io>
@ 2018-05-28 12:49 ` Julio Guerra
2018-05-28 13:18 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Julio Guerra @ 2018-05-28 12:49 UTC (permalink / raw)
To: Corinna Vinschen, Pedro Alves; +Cc: gdb-patches
> 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.
--
Julio Guerra
Co-founder & CTO of Farjump
Mobile: +33 618 644 164
LinkedIn: https://linkedin.com/in/guerrajulio
Slack: farjump.slack.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Do not clear the value of st_dev in File I/O's stat()
2018-05-28 12:49 ` Julio Guerra
@ 2018-05-28 13:18 ` Pedro Alves
0 siblings, 0 replies; 6+ messages in thread
From: Pedro Alves @ 2018-05-28 13:18 UTC (permalink / raw)
To: Julio Guerra, Corinna Vinschen; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-28 12:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20180517082631.26855-1-julio@farjump.io>
2018-05-17 10:32 ` [PATCH] Do not clear the value of st_dev in File I/O's stat() Julio Guerra
2018-05-17 15:31 ` Pedro Alves
[not found] ` <496efebf-0f96-4586-de39-0a1857994f04@farjump.io>
2018-05-18 18:29 ` Julio Guerra
2018-05-28 10:50 ` Corinna Vinschen
[not found] ` <6f5980ae-247c-1991-ba3c-884fe04a94c5@farjump.io>
2018-05-28 12:49 ` Julio Guerra
2018-05-28 13:18 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).