* [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
[parent not found: <496efebf-0f96-4586-de39-0a1857994f04@farjump.io>]
* 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
[parent not found: <6f5980ae-247c-1991-ba3c-884fe04a94c5@farjump.io>]
* 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).