[v3] Allow using special files with File I/O functions

Message ID 0102016447dcf9e9-3989bcd9-1272-4a05-93c5-77823c7a0921-000000@eu-west-1.amazonses.com
State New
Headers show
Series
  • [v3] Allow using special files with File I/O functions
Related show

Commit Message

Julio Guerra June 28, 2018, 7:27 p.m.
- Remove the restriction to regular files only and add support for special file

types in the File IO stat structure.

- Define a few more macro definitions of file types such as FIFOs, etc.



The major goal is being able to write advanced embedded testing functions, like:

- using a FIFO between the embedded program and the host, instead of being

  restricted only to the GDB console.

- mocking features based on host's by opening some /dev special files.



2018-06-28  Julio Guerra  <julio@farjump.io>



        * remote-fileio.c (remote_fileio_func_open, remote_fileio_func_stat):

        Allow using File I/O functions open(), stat() and fstat() on special

        files.

        * ../include/gdb/fileio.h: Add macro definitions for special files,

        both for fst_dev and fst_mode fields of struct fst_stat.

        * common/fileio.c (fileio_to_host_mode, fileio_mode_pack): Add new

        special file types in fst_mode's definition.

        (host_to_fileio_stat): Define fst_dev using the new macro definitions

        and according to the file's type.



Signed-off-by: Julio Guerra <julio@farjump.io>


---

 gdb/ChangeLog        | 12 ++++++++

 gdb/common/fileio.c  | 66 +++++++++++++++++++++++++++++++++++---------

 gdb/remote-fileio.c  | 28 ++-----------------

 include/gdb/fileio.h | 19 +++++++++++--

 4 files changed, 83 insertions(+), 42 deletions(-)



-- 

2.18.0

Comments

Julio Guerra June 28, 2018, 7:30 p.m. | #1
You can find below the logs of the test I performed using our GDB server with a program embedded in a Raspberry Pi:

 

(gdb) set $st = (struct stat*) malloc(sizeof (struct stat))

(gdb) call stat("/dev/stdout", $st)
$1 = 0x0
(gdb) p *$st
$2 = {
  st_dev = 0x2, 
  st_ino = 0x4, 
  st_mode = 0x2190, 
  st_nlink = 0x1, 
  st_uid = 0x3e8, 
  st_gid = 0x5, 
  st_rdev = 0x8801, 
  st_size = 0x0, 
  st_atime = 0x5b3507e0, 
  st_spare1 = 0x0, 
  st_mtime = 0x5b34fb47, 
  st_spare2 = 0x0, 
  st_ctime = 0x0, 
  st_spare3 = 0x0, 
  st_blksize = 0x0, 
  st_blocks = 0x5b3507e0, 
  st_spare4 = {0x0, 0x0}
}
(gdb) p /o $st->st_mode            
$8 = 020620

(gdb) call stat("myfifo", $st)     
$3 = 0x0
(gdb) p *$st
$4 = {
  st_dev = 0x2, 
  st_ino = 0x4f7c, 
  st_mode = 0x11a4, 
  st_nlink = 0x1, 
  st_uid = 0x3e8, 
  st_gid = 0x3e8, 
  st_rdev = 0x0, 
  st_size = 0x0, 
  st_atime = 0x5b3504ed, 
  st_spare1 = 0x0, 
  st_mtime = 0x5b3504ed, 
  st_spare2 = 0x0, 
  st_ctime = 0x0, 
  st_spare3 = 0x0, 
  st_blksize = 0x0, 
  st_blocks = 0x5b3504ed, 
  st_spare4 = {0x0, 0x0}
}
(gdb) p /o $st->st_mode       
$6 = 010644

(gdb) call stat("/run/dbus/system_bus_socket", $st)  
$9 = 0x0
(gdb) p *$st                                       
$10 = {
  st_dev = 0x2, 
  st_ino = 0x36c7, 
  st_mode = 0xc1b6, 
  st_nlink = 0x1, 
  st_uid = 0x0, 
  st_gid = 0x0, 
  st_rdev = 0x0, 
  st_size = 0x0, 
  st_atime = 0x5b33e995, 
  st_spare1 = 0x0, 
  st_mtime = 0x5b33e995, 
  st_spare2 = 0x0, 
  st_ctime = 0x0, 
  st_spare3 = 0x0, 
  st_blksize = 0x0, 
  st_blocks = 0x5b33e995, 
  st_spare4 = {0x0, 0x0}
}
(gdb) p /o $st->st_mode                            
$11 = 0140666

(gdb) call stat("/tmp", $st)                       
$12 = 0x0
(gdb) p *$st                
$13 = {
  st_dev = 0x2, 
  st_ino = 0x7a5, 
  st_mode = 0x41ff, 
  st_nlink = 0x1, 
  st_uid = 0x0, 
  st_gid = 0x0, 
  st_rdev = 0x0, 
  st_size = 0x0, 
  st_atime = 0x5b350829, 
  st_spare1 = 0x0, 
  st_mtime = 0x5b350829, 
  st_spare2 = 0x0, 
  st_ctime = 0x0, 
  st_spare3 = 0x0, 
  st_blksize = 0x0, 
  st_blocks = 0x5a5a9802, 
  st_spare4 = {0x0, 0x0}
}
(gdb) p /o $st->st_mode     
$14 = 040777

(gdb) call stat("Makefile", $st)
$15 = 0x0
(gdb) p *$st                    
$16 = {
  st_dev = 0x0, 
  st_ino = 0xc6b9, 
  st_mode = 0x81a4, 
  st_nlink = 0x1, 
  st_uid = 0x3e8, 
  st_gid = 0x3e8, 
  st_rdev = 0x0, 
  st_size = 0x0, 
  st_atime = 0x5ae35b23, 
  st_spare1 = 0x0, 
  st_mtime = 0x5b311f46, 
  st_spare2 = 0x0, 
  st_ctime = 0x0, 
  st_spare3 = 0x0, 
  st_blksize = 0x0, 
  st_blocks = 0x5b3500d5, 
  st_spare4 = {0x0, 0x0}
}
(gdb) p /o $st->st_mode         
$17 = 0100644

(gdb) call stat("i dont exist", $st)
$18 = 0xffffffff

(gdb) call fstat(0, $st)
$1 = 0x0
(gdb) p *$st
$3 = {
  st_dev = 0x1, 
  st_ino = 0x0, 
  st_mode = 0x2100, 
  st_nlink = 0x1, 
  st_uid = 0x3e8, 
  st_gid = 0x3e8, 
  st_rdev = 0x0, 
  st_size = 0x0, 
  st_atime = 0x5b35144e, 
  st_spare1 = 0x0, 
  st_mtime = 0x5b35144e, 
  st_spare2 = 0x0, 
  st_ctime = 0x0, 
  st_spare3 = 0x0, 
  st_blksize = 0x0, 
  st_blocks = 0x5b35144e, 
  st_spare4 = {0x0, 0x0}
}

(gdb) call fstat(1, $st)
$4 = 0x0
(gdb) p *$st                                               
$5 = {
  st_dev = 0x1, 
  st_ino = 0x0, 
  st_mode = 0x2080, 
  st_nlink = 0x1, 
  st_uid = 0x3e8, 
  st_gid = 0x3e8, 
  st_rdev = 0x0, 
  st_size = 0x0, 
  st_atime = 0x5b351460, 
  st_spare1 = 0x0, 
  st_mtime = 0x5b351460, 
  st_spare2 = 0x0, 
  st_ctime = 0x0, 
  st_spare3 = 0x0, 
  st_blksize = 0x0, 
  st_blocks = 0x5b351460, 
  st_spare4 = {0x0, 0x0}
}

(gdb) call fstat(2, $st)
$6 = 0x0
(gdb) p *$st            
$7 = {
  st_dev = 0x1, 
  st_ino = 0x0, 
  st_mode = 0x2080, 
  st_nlink = 0x1, 
  st_uid = 0x3e8, 
  st_gid = 0x3e8, 
  st_rdev = 0x0, 
  st_size = 0x0, 
  st_atime = 0x5b35146b, 
  st_spare1 = 0x0, 
  st_mtime = 0x5b35146b, 
  st_spare2 = 0x0, 
  st_ctime = 0x0, 
  st_spare3 = 0x0, 
  st_blksize = 0x0, 
  st_blocks = 0x5b35146b, 
  st_spare4 = {0x0, 0x0}
}

(gdb) call fstat(3, $st)
$8 = 0xffffffff

(gdb) set $fd = open("myfifo", 0)
(gdb) call fstat($fd, $st)       
$10 = 0x0
(gdb) p *$st
$12 = {
  st_dev = 0x2, 
  st_ino = 0x4f7c, 
  st_mode = 0x11a4, 
  st_nlink = 0x1, 
  st_uid = 0x3e8, 
  st_gid = 0x3e8, 
  st_rdev = 0x0, 
  st_size = 0x0, 
  st_atime = 0x5b351511, 
  st_spare1 = 0x0, 
  st_mtime = 0x5b351511, 
  st_spare2 = 0x0, 
  st_ctime = 0x0, 
  st_spare3 = 0x0, 
  st_blksize = 0x0, 
  st_blocks = 0x5b3504ed, 
  st_spare4 = {0x0, 0x0}
}
(gdb) p /o $st->st_mode
$13 = 010644


 ​
 
 

-- 
Julio Guerra
Co-founder & CTO of Farjump
Mobile: +33 618 644 164
LinkedIn: https://linkedin.com/in/guerrajulio
Slack: farjump.slack.com
Pedro Alves June 29, 2018, 1:42 p.m. | #2
On 06/28/2018 08:27 PM, Julio Guerra wrote:
> - Remove the restriction to regular files only and add support for special file

> types in the File IO stat structure.

> - Define a few more macro definitions of file types such as FIFOs, etc.

> 

> The major goal is being able to write advanced embedded testing functions, like:

> - using a FIFO between the embedded program and the host, instead of being

>   restricted only to the GDB console.

> - mocking features based on host's by opening some /dev special files.

> 


This needs a GDB manual change, and a NEWS entry.

> 2018-06-28  Julio Guerra  <julio@farjump.io>

> 

>         * remote-fileio.c (remote_fileio_func_open, remote_fileio_func_stat):

>         Allow using File I/O functions open(), stat() and fstat() on special

>         files.

>         * ../include/gdb/fileio.h: Add macro definitions for special files,

>         both for fst_dev and fst_mode fields of struct fst_stat.

>         * common/fileio.c (fileio_to_host_mode, fileio_mode_pack): Add new

>         special file types in fst_mode's definition.

>         (host_to_fileio_stat): Define fst_dev using the new macro definitions

>         and according to the file's type.


Note that include/ has its own ChangeLog file.

>  2018-06-27  Simon Marchi  <simon.marchi@ericsson.com>

>  

>  	* gdb-gdb.py.in (StructMainTypePrettyPrinter) <to_string>: Don't

> diff --git a/gdb/common/fileio.c b/gdb/common/fileio.c

> index 912a7ede3c..9ee78e227c 100644

> --- a/gdb/common/fileio.c

> +++ b/gdb/common/fileio.c

> @@ -119,12 +119,31 @@ fileio_to_host_mode (int fileio_mode, mode_t *mode_p)

>    if (fileio_mode & ~FILEIO_S_SUPPORTED)

>      return -1;

>  

> -  if (fileio_mode & FILEIO_S_IFREG)

> -    mode |= S_IFREG;

> -  if (fileio_mode & FILEIO_S_IFDIR)

> -    mode |= S_IFDIR;

> -  if (fileio_mode & FILEIO_S_IFCHR)

> -    mode |= S_IFCHR;

> +  switch (fileio_mode & FILEIO_S_IFMT)

> +    {

> +    case FILEIO_S_IFSOCK:

> +      *mode_p |= S_IFSOCK;

> +      break;

> +    case FILEIO_S_IFLNK:

> +      mode |= S_IFLNK;

> +      break;

> +    case FILEIO_S_IFREG:

> +      mode |= S_IFREG;

> +      break;

> +    case FILEIO_S_IFBLK:

> +      mode |= S_IFBLK;

> +      break;

> +    case FILEIO_S_IFDIR:

> +      mode |= S_IFDIR;

> +      break;

> +    case FILEIO_S_IFCHR:

> +      mode |= S_IFCHR;

> +      break;

> +    case FILEIO_S_IFIFO:

> +      mode |= S_IFIFO;

> +      break;

> +    }

> +

>    if (fileio_mode & FILEIO_S_IRUSR)

>      mode |= S_IRUSR;

>    if (fileio_mode & FILEIO_S_IWUSR)

> @@ -165,12 +184,31 @@ fileio_mode_pack (mode_t mode)

>  {

>    mode_t tmode = 0;

>  

> -  if (S_ISREG (mode))

> -    tmode |= FILEIO_S_IFREG;

> -  if (S_ISDIR (mode))

> -    tmode |= FILEIO_S_IFDIR;

> -  if (S_ISCHR (mode))

> -    tmode |= FILEIO_S_IFCHR;

> +  switch (mode & S_IFMT)

> +    {

> +    case S_IFSOCK:

> +      tmode |= FILEIO_S_IFSOCK;

> +      break;

> +    case S_IFLNK:

> +      tmode |= FILEIO_S_IFLNK;

> +      break;

> +    case S_IFREG:

> +      tmode |= FILEIO_S_IFREG;

> +      break;

> +    case S_IFBLK:

> +      tmode |= FILEIO_S_IFBLK;

> +      break;

> +    case S_IFDIR:

> +      tmode |= FILEIO_S_IFDIR;

> +      break;

> +    case S_IFCHR:

> +      tmode |= FILEIO_S_IFCHR;

> +      break;

> +    case S_IFIFO:

> +      tmode |= FILEIO_S_IFIFO;

> +      break;

> +    }


I'm not sure whether all these S_FOO macros exist
on all hosts.

Looking at:
 https://www.gnu.org/software/gnulib/manual/html_node/sys_002fstat_002eh.html

gnulib's sys/stat.h (gdb/gnulib/import/sys_stat.in.h) adds some
replacements, but then I'm not sure whether checking S_IFxxx
etc. directly instead of using the S_ISxxx function-style macros
is the right thing to do portability-wise.  It may be S_ISxxx was being
used for a reason?

> +

>    if (mode & S_IRUSR)

>      tmode |= FILEIO_S_IRUSR;

>    if (mode & S_IWUSR)

> @@ -224,8 +262,10 @@ void

>  host_to_fileio_stat (struct stat *st, struct fio_stat *fst)

>  {

>    LONGEST blksize;

> +  long    fst_dev;


We don't align variable names like that.

>  

> -  host_to_fileio_uint ((long) st->st_dev, fst->fst_dev);

> +  fst_dev = S_ISREG(st->st_mode) ? FILEIO_STDEV_FILE : FILEIO_STDEV_SPECIAL;


Missing space in "S_ISREG (".

> +  host_to_fileio_uint (fst_dev, fst->fst_dev);

>    host_to_fileio_uint ((long) st->st_ino, fst->fst_ino);

>    host_to_fileio_mode (st->st_mode, fst->fst_mode);

>    host_to_fileio_uint ((long) st->st_nlink, fst->fst_nlink);

> diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c

> index 313da642ea..837081269a 100644

> --- a/gdb/remote-fileio.c

> +++ b/gdb/remote-fileio.c

> @@ -407,24 +407,6 @@ remote_fileio_func_open (remote_target *remote, char *buf)

>        return;

>      }

>  

> -  /* Check if pathname exists and is not a regular file or directory.  If so,

> -     return an appropriate error code.  Same for trying to open directories

> -     for writing.  */


Did you intend to remove the "Same for trying to open directories
for writing." part?

Thanks,
Pedro Alves
Julio Guerra June 29, 2018, 2:01 p.m. | #3
Le 29/06/2018 à 15:42, Pedro Alves a écrit :

 


 
 


 

On 06/28/2018 08:27 PM, Julio Guerra wrote:


 

- Remove the restriction to regular files only and add support for special file
types in the File IO stat structure.
- Define a few more macro definitions of file types such as FIFOs, etc.

The major goal is being able to write advanced embedded testing functions, like:
- using a FIFO between the embedded program and the host, instead of being
  restricted only to the GDB console.
- mocking features based on host's by opening some /dev special files.



 

This needs a GDB manual change, and a NEWS entry.

 


 
 


 

Ok.

 


 
 


 




 

2018-06-28  Julio Guerra  <julio@farjump.io>

        * remote-fileio.c (remote_fileio_func_open, remote_fileio_func_stat):
        Allow using File I/O functions open(), stat() and fstat() on special
        files.
        * ../include/gdb/fileio.h: Add macro definitions for special files,
        both for fst_dev and fst_mode fields of struct fst_stat.
        * common/fileio.c (fileio_to_host_mode, fileio_mode_pack): Add new
        special file types in fst_mode's definition.
        (host_to_fileio_stat): Define fst_dev using the new macro definitions
        and according to the file's type.


 

Note that include/ has its own ChangeLog file.

 


 
 


 

Ok.

 


 
 


 




 

 2018-06-27  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* gdb-gdb.py.in (StructMainTypePrettyPrinter) <to_string>: Don't
diff --git a/gdb/common/fileio.c b/gdb/common/fileio.c
index 912a7ede3c..9ee78e227c 100644
--- a/gdb/common/fileio.c
+++ b/gdb/common/fileio.c
@@ -119,12 +119,31 @@ fileio_to_host_mode (int fileio_mode, mode_t *mode_p)
   if (fileio_mode & ~FILEIO_S_SUPPORTED)
     return -1;
 
-  if (fileio_mode & FILEIO_S_IFREG)
-    mode |= S_IFREG;
-  if (fileio_mode & FILEIO_S_IFDIR)
-    mode |= S_IFDIR;
-  if (fileio_mode & FILEIO_S_IFCHR)
-    mode |= S_IFCHR;
+  switch (fileio_mode & FILEIO_S_IFMT)
+    {
+    case FILEIO_S_IFSOCK:
+      *mode_p |= S_IFSOCK;
+      break;
+    case FILEIO_S_IFLNK:
+      mode |= S_IFLNK;
+      break;
+    case FILEIO_S_IFREG:
+      mode |= S_IFREG;
+      break;
+    case FILEIO_S_IFBLK:
+      mode |= S_IFBLK;
+      break;
+    case FILEIO_S_IFDIR:
+      mode |= S_IFDIR;
+      break;
+    case FILEIO_S_IFCHR:
+      mode |= S_IFCHR;
+      break;
+    case FILEIO_S_IFIFO:
+      mode |= S_IFIFO;
+      break;
+    }
+
   if (fileio_mode & FILEIO_S_IRUSR)
     mode |= S_IRUSR;
   if (fileio_mode & FILEIO_S_IWUSR)
@@ -165,12 +184,31 @@ fileio_mode_pack (mode_t mode)
 {
   mode_t tmode = 0;
 
-  if (S_ISREG (mode))
-    tmode |= FILEIO_S_IFREG;
-  if (S_ISDIR (mode))
-    tmode |= FILEIO_S_IFDIR;
-  if (S_ISCHR (mode))
-    tmode |= FILEIO_S_IFCHR;
+  switch (mode & S_IFMT)
+    {
+    case S_IFSOCK:
+      tmode |= FILEIO_S_IFSOCK;
+      break;
+    case S_IFLNK:
+      tmode |= FILEIO_S_IFLNK;
+      break;
+    case S_IFREG:
+      tmode |= FILEIO_S_IFREG;
+      break;
+    case S_IFBLK:
+      tmode |= FILEIO_S_IFBLK;
+      break;
+    case S_IFDIR:
+      tmode |= FILEIO_S_IFDIR;
+      break;
+    case S_IFCHR:
+      tmode |= FILEIO_S_IFCHR;
+      break;
+    case S_IFIFO:
+      tmode |= FILEIO_S_IFIFO;
+      break;
+    }


 

I'm not sure whether all these S_FOO macros exist
on all hosts.

Looking at:
 https://www.gnu.org/software/gnulib/manual/html_node/sys_002fstat_002eh.html

gnulib's sys/stat.h (gdb/gnulib/import/sys_stat.in.h) adds some
replacements, but then I'm not sure whether checking S_IFxxx
etc. directly instead of using the S_ISxxx function-style macros
is the right thing to do portability-wise.  It may be S_ISxxx was being
used for a reason?


 


 
 


 

I assumed a system >= POSIX.1-2001 here. man 7 inode says:

 

The S_IF* constants are present in POSIX.1-2001 and later.
 […]
 The S_ISLNK() and S_ISSOCK() macros were not in POSIX.1-1996, but
 both are present in POSIX.1-2001

 


 
 


 


 

+
   if (mode & S_IRUSR)
     tmode |= FILEIO_S_IRUSR;
   if (mode & S_IWUSR)
@@ -224,8 +262,10 @@ void
 host_to_fileio_stat (struct stat *st, struct fio_stat *fst)
 {
   LONGEST blksize;
+  long    fst_dev;


 

We don't align variable names like that.


 


 
 


 

Ok.

 


 
 


 


 

 
-  host_to_fileio_uint ((long) st->st_dev, fst->fst_dev);
+  fst_dev = S_ISREG(st->st_mode) ? FILEIO_STDEV_FILE : FILEIO_STDEV_SPECIAL;


 

Missing space in "S_ISREG (".

 


 
 


 

Ok.

 


 
 


 




 

+  host_to_fileio_uint (fst_dev, fst->fst_dev);
   host_to_fileio_uint ((long) st->st_ino, fst->fst_ino);
   host_to_fileio_mode (st->st_mode, fst->fst_mode);
   host_to_fileio_uint ((long) st->st_nlink, fst->fst_nlink);
diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 313da642ea..837081269a 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -407,24 +407,6 @@ remote_fileio_func_open (remote_target *remote, char *buf)
       return;
     }
 
-  /* Check if pathname exists and is not a regular file or directory.  If so,
-     return an appropriate error code.  Same for trying to open directories
-     for writing.  */


 

Did you intend to remove the "Same for trying to open directories
for writing." part?

 


 
 


 

Yes, because of man 2 open:

 

EISDIR: The named file is a directory and oflag includes O_WRONLY or O_RDWR,
 or includes O_CREAT without O_DIRECTORY.

 

I wait for your comments until I resubmit a v4.

 

Thank you,
 

 ​
 
 

-- 
Julio Guerra
Pedro Alves June 29, 2018, 2:28 p.m. | #4
Hi,

Your message came our hard to read:

 https://sourceware.org/ml/gdb-patches/2018-06/msg00715.html

Please make sure your client is set up to quote replies.

On 06/29/2018 03:01 PM, Julio Guerra wrote:

> I assumed a system >= POSIX.1-2001 here. man 7 inode says:

> 

>  

> 

> The S_IF* constants are present in POSIX.1-2001 and later.

>  […]

>  The S_ISLNK() and S_ISSOCK() macros were not in POSIX.1-1996, but

>  both are present in POSIX.1-2001

> 

> 


POSIX specification != actual implementations.  Also, Windows != POSIX,
for example.  See the gnulib page I pointed at.  Also, looking through
history with "git blame", etc. may find something.

> Yes, because of man 2 open:

> 

>  

> 

> EISDIR: The named file is a directory and oflag includes O_WRONLY or O_RDWR,

>  or includes O_CREAT without O_DIRECTORY.


I assume you're on Linux, so "man 2" is the Linux Programmer's manual.
But GDB works in other hosts too.  So it may be the code was there for
some other host.  I mean, why did someone write that in the first place?
Again, sounds like some code archaeology is in order.

I forgot to say in the previous email, but it would be really nice if we
could add some coverage for these change to the testsuite.  I've asked
before but I don't remember the answer -- Would it be possible to portably
update e.g. gdb.base/fileio.exp to cover at least one kind
of FILEIO_STDEV_SPECIAL file?

Thanks,
Pedro Alves
Julio Guerra June 29, 2018, 2:40 p.m. | #5
Le 29/06/2018 à 16:28, Pedro Alves a écrit :

> Hi,


>


> Your message came our hard to read:


>


>  https://sourceware.org/ml/gdb-patches/2018-06/msg00715.html


>


> Please make sure your client is set up to quote replies.


>


> On 06/29/2018 03:01 PM, Julio Guerra wrote:


>


>> I assumed a system >= POSIX.1-2001 here. man 7 inode says:


>>


>>  


>>


>> The S_IF* constants are present in POSIX.1-2001 and later.


>>  […]


>>  The S_ISLNK() and S_ISSOCK() macros were not in POSIX.1-1996, but


>>  both are present in POSIX.1-2001


>>


>>


> POSIX specification != actual implementations.  Also, Windows != POSIX,


> for example.  See the gnulib page I pointed at.  Also, looking through


> history with "git blame", etc. may find something.




Isn't GDB compiled with mingw? I am no expert in mingw, but I thought it

was a POSIX implementation based on Windows.

Anyway, we can add usual #ifdef guards arounds the cases.

>


>> Yes, because of man 2 open:


>>


>>  


>>


>> EISDIR: The named file is a directory and oflag includes O_WRONLY or O_RDWR,


>>  or includes O_CREAT without O_DIRECTORY.


> I assume you're on Linux, so "man 2" is the Linux Programmer's manual.


> But GDB works in other hosts too.  So it may be the code was there for


> some other host.  I mean, why did someone write that in the first place?


> Again, sounds like some code archaeology is in order.




Yes, but I checked it was POSIX too:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

>


> I forgot to say in the previous email, but it would be really nice if we


> could add some coverage for these change to the testsuite.  I've asked


> before but I don't remember the answer -- Would it be possible to portably


> update e.g. gdb.base/fileio.exp to cover at least one kind


> of FILEIO_STDEV_SPECIAL file?




Ok, I'll have a look, but I thought there File IOs were not implemented

in gdbserver, so what runs this test? If it is natively run, it won't

cover remote_fileio_func_*.



-- 

Julio Guerra

Co-founder & CTO of Farjump

Mobile: +33 618 644 164

LinkedIn: https://linkedin.com/in/guerrajulio

Slack: farjump.slack.com
Pedro Alves June 29, 2018, 3 p.m. | #6
On 06/29/2018 03:40 PM, Julio Guerra wrote:
> Le 29/06/2018 à 16:28, Pedro Alves a écrit :

>> On 06/29/2018 03:01 PM, Julio Guerra wrote:

>>

>>> I assumed a system >= POSIX.1-2001 here. man 7 inode says:

>>>

>>>  

>>>

>>> The S_IF* constants are present in POSIX.1-2001 and later.

>>>  […]

>>>  The S_ISLNK() and S_ISSOCK() macros were not in POSIX.1-1996, but

>>>  both are present in POSIX.1-2001

>>>

>>>

>> POSIX specification != actual implementations.  Also, Windows != POSIX,

>> for example.  See the gnulib page I pointed at.  Also, looking through

>> history with "git blame", etc. may find something.

> 

> Isn't GDB compiled with mingw? I am no expert in mingw, but I thought it

> was a POSIX implementation based on Windows.


It's compiled with either MinGW or Cygwin.  Cygwin is POSIX-like, but
mingw is native Windows.

> Anyway, we can add usual #ifdef guards arounds the cases.


That may not be needed, but does not address the S_ISxxx vs S_IFxxx comment.
Again, please take a look at the gnulib header.  ISTM that S_ISxxx is
always available (there's a default implementation that just returns 0),
not sure about S_IFxxx.  But please do take a better look.  I've only
skimmed it.

>>

>>> Yes, because of man 2 open:

>>>

>>>  

>>>

>>> EISDIR: The named file is a directory and oflag includes O_WRONLY or O_RDWR,

>>>  or includes O_CREAT without O_DIRECTORY.

>> I assume you're on Linux, so "man 2" is the Linux Programmer's manual.

>> But GDB works in other hosts too.  So it may be the code was there for

>> some other host.  I mean, why did someone write that in the first place?

>> Again, sounds like some code archaeology is in order.

> 

> Yes, but I checked it was POSIX too:

> http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html


Again, spec vs implementation.  It may not be needed, but I wouldn't be
surprised if that was needed on e.g., Windows either.  Again, archaeology
may help here.

>>

>> I forgot to say in the previous email, but it would be really nice if we

>> could add some coverage for these change to the testsuite.  I've asked

>> before but I don't remember the answer -- Would it be possible to portably

>> update e.g. gdb.base/fileio.exp to cover at least one kind

>> of FILEIO_STDEV_SPECIAL file?

> 

> Ok, I'll have a look, but I thought there File IOs were not implemented

> in gdbserver, so what runs this test? If it is natively run, it won't

> cover remote_fileio_func_*.


You can run the testsuite against other remote stubs, not just gdbserver.
Ideally, you'd set up the testsuite to against your stub.  Did you try
that?  I'd recommend that regardless.

Otherwise, even if we only test it when natively run, other folks that have
embedded stubs that support fileio will end up exercising the test.

Thanks,
Pedro Alves
Julio Guerra July 4, 2018, 9:44 a.m. | #7
Pedro,

> You can run the testsuite against other remote stubs, not just gdbserver.

> Ideally, you'd set up the testsuite to against your stub.  Did you try

> that?  I'd recommend that regardless.

>

> Otherwise, even if we only test it when natively run, other folks that have

> embedded stubs that support fileio will end up exercising the test.


I have almost finished writing the tests, but I am struggling on how to
create some special files:
- Creating block or character devices require mknod with root privileges.
- Creating a unix socket is not supported by the TCL library, and
require extra tools (netcat, socat...) to create one from the command line.

Here is what I use for now for the other types:
- Link: TCL function `file link`
- Regular: TCL function `open`
- FIFO pipe command line: `mknod myfifo p` or `mkfifo myfifo` which
doesn't exist on Windows.

What do you suggest? Should I avoid every non portable test cases, which
limits the tests only to links, regular files and directories?

-- 
Julio Guerra
Pedro Alves July 5, 2018, 4:50 p.m. | #8
On 07/04/2018 10:44 AM, Julio Guerra wrote:

>> You can run the testsuite against other remote stubs, not just gdbserver.

>> Ideally, you'd set up the testsuite to against your stub.  Did you try

>> that?  I'd recommend that regardless.

>>

>> Otherwise, even if we only test it when natively run, other folks that have

>> embedded stubs that support fileio will end up exercising the test.

> 

> I have almost finished writing the tests, but I am struggling on how to

> create some special files:

> - Creating block or character devices require mknod with root privileges.

> - Creating a unix socket is not supported by the TCL library, and

> require extra tools (netcat, socat...) to create one from the command line.

> 

> Here is what I use for now for the other types:

> - Link: TCL function `file link`

> - Regular: TCL function `open`

> - FIFO pipe command line: `mknod myfifo p` or `mkfifo myfifo` which

> doesn't exist on Windows.

> 

> What do you suggest? Should I avoid every non portable test cases, which

> limits the tests only to links, regular files and directories?


Yeah, as long as we test at least one "special" file, I think it's OK.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog

index 18c1915675..19cf5e38a6 100644

--- a/gdb/ChangeLog

+++ b/gdb/ChangeLog

@@ -1,3 +1,15 @@ 

+2018-06-28  Julio Guerra  <julio@farjump.io>

+

+	* remote-fileio.c (remote_fileio_func_open, remote_fileio_func_stat):

+	Allow using File I/O functions open(), stat() and fstat() on special

+	files.

+	* ../include/gdb/fileio.h: Add macro definitions for special files,

+	both for fst_dev and fst_mode fields of struct fst_stat.

+	* common/fileio.c (fileio_to_host_mode, fileio_mode_pack): Add new

+	special file types in fst_mode's definition.

+	(host_to_fileio_stat): Define fst_dev using the new macro definitions

+	and according to the file's type.

+

 2018-06-27  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* gdb-gdb.py.in (StructMainTypePrettyPrinter) <to_string>: Don't
diff --git a/gdb/common/fileio.c b/gdb/common/fileio.c

index 912a7ede3c..9ee78e227c 100644

--- a/gdb/common/fileio.c

+++ b/gdb/common/fileio.c

@@ -119,12 +119,31 @@  fileio_to_host_mode (int fileio_mode, mode_t *mode_p)

   if (fileio_mode & ~FILEIO_S_SUPPORTED)
     return -1;
 
-  if (fileio_mode & FILEIO_S_IFREG)

-    mode |= S_IFREG;

-  if (fileio_mode & FILEIO_S_IFDIR)

-    mode |= S_IFDIR;

-  if (fileio_mode & FILEIO_S_IFCHR)

-    mode |= S_IFCHR;

+  switch (fileio_mode & FILEIO_S_IFMT)

+    {

+    case FILEIO_S_IFSOCK:

+      *mode_p |= S_IFSOCK;

+      break;

+    case FILEIO_S_IFLNK:

+      mode |= S_IFLNK;

+      break;

+    case FILEIO_S_IFREG:

+      mode |= S_IFREG;

+      break;

+    case FILEIO_S_IFBLK:

+      mode |= S_IFBLK;

+      break;

+    case FILEIO_S_IFDIR:

+      mode |= S_IFDIR;

+      break;

+    case FILEIO_S_IFCHR:

+      mode |= S_IFCHR;

+      break;

+    case FILEIO_S_IFIFO:

+      mode |= S_IFIFO;

+      break;

+    }

+

   if (fileio_mode & FILEIO_S_IRUSR)
     mode |= S_IRUSR;
   if (fileio_mode & FILEIO_S_IWUSR)
@@ -165,12 +184,31 @@  fileio_mode_pack (mode_t mode)

 {
   mode_t tmode = 0;
 
-  if (S_ISREG (mode))

-    tmode |= FILEIO_S_IFREG;

-  if (S_ISDIR (mode))

-    tmode |= FILEIO_S_IFDIR;

-  if (S_ISCHR (mode))

-    tmode |= FILEIO_S_IFCHR;

+  switch (mode & S_IFMT)

+    {

+    case S_IFSOCK:

+      tmode |= FILEIO_S_IFSOCK;

+      break;

+    case S_IFLNK:

+      tmode |= FILEIO_S_IFLNK;

+      break;

+    case S_IFREG:

+      tmode |= FILEIO_S_IFREG;

+      break;

+    case S_IFBLK:

+      tmode |= FILEIO_S_IFBLK;

+      break;

+    case S_IFDIR:

+      tmode |= FILEIO_S_IFDIR;

+      break;

+    case S_IFCHR:

+      tmode |= FILEIO_S_IFCHR;

+      break;

+    case S_IFIFO:

+      tmode |= FILEIO_S_IFIFO;

+      break;

+    }

+

   if (mode & S_IRUSR)
     tmode |= FILEIO_S_IRUSR;
   if (mode & S_IWUSR)
@@ -224,8 +262,10 @@  void

 host_to_fileio_stat (struct stat *st, struct fio_stat *fst)
 {
   LONGEST blksize;
+  long    fst_dev;

 
-  host_to_fileio_uint ((long) st->st_dev, fst->fst_dev);

+  fst_dev = S_ISREG(st->st_mode) ? FILEIO_STDEV_FILE : FILEIO_STDEV_SPECIAL;

+  host_to_fileio_uint (fst_dev, fst->fst_dev);

   host_to_fileio_uint ((long) st->st_ino, fst->fst_ino);
   host_to_fileio_mode (st->st_mode, fst->fst_mode);
   host_to_fileio_uint ((long) st->st_nlink, fst->fst_nlink);
diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c

index 313da642ea..837081269a 100644

--- a/gdb/remote-fileio.c

+++ b/gdb/remote-fileio.c

@@ -407,24 +407,6 @@  remote_fileio_func_open (remote_target *remote, char *buf)

       return;
     }
 
-  /* Check if pathname exists and is not a regular file or directory.  If so,

-     return an appropriate error code.  Same for trying to open directories

-     for writing.  */

-  if (!stat (pathname, &st))

-    {

-      if (!S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))

-	{

-	  remote_fileio_reply (remote, -1, FILEIO_ENODEV);

-	  return;

-	}

-      if (S_ISDIR (st.st_mode)

-	  && ((flags & O_WRONLY) == O_WRONLY || (flags & O_RDWR) == O_RDWR))

-	{

-	  remote_fileio_reply (remote, -1, FILEIO_EISDIR);

-	  return;

-	}

-    }

-

   fd = gdb_open_cloexec (pathname, flags, mode);
   if (fd < 0)
     {
@@ -885,16 +867,9 @@  remote_fileio_func_stat (remote_target *remote, char *buf)

       remote_fileio_return_errno (remote, -1);
       return;
     }
-  /* Only operate on regular files and directories.  */

-  if (!ret && !S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))

-    {

-      remote_fileio_reply (remote, -1, FILEIO_EACCES);

-      return;

-    }

   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)
@@ -939,7 +914,6 @@  remote_fileio_func_fstat (remote_target *remote, char *buf)

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

       memset (&st, 0, sizeof (st));
       st.st_mode = S_IFCHR | (fd == FIO_FD_CONSOLE_IN ? S_IRUSR : S_IWUSR);
       st.st_nlink = 1;
@@ -972,6 +946,8 @@  remote_fileio_func_fstat (remote_target *remote, char *buf)

   if (ptrval)
     {
       host_to_fileio_stat (&st, &fst);
+      if (fd == FIO_FD_CONSOLE_IN || fd == FIO_FD_CONSOLE_OUT)

+	host_to_fileio_uint (FILEIO_STDEV_CONSOLE, fst.fst_dev);

 
       errno = target_write_memory (ptrval, (gdb_byte *) &fst, sizeof fst);
       if (errno != 0)
diff --git a/include/gdb/fileio.h b/include/gdb/fileio.h

index 7bb55f579f..ada84c5f10 100644

--- a/include/gdb/fileio.h

+++ b/include/gdb/fileio.h

@@ -37,10 +37,24 @@ 

 				 FILEIO_O_CREAT  | FILEIO_O_TRUNC| \
 				 FILEIO_O_EXCL)
 
+/* Device id values of fst_dev field */

+/* Regular file */

+#define FILEIO_STDEV_FILE           0

+/* GDB's console */

+#define FILEIO_STDEV_CONSOLE        1

+/* Not a regular file nor the console.

+   Bits FILEIO_S_IFMT of fst_mode give the exact file type. */

+#define FILEIO_STDEV_SPECIAL        2

+

 /* mode_t bits */
+#define FILEIO_S_IFSOCK       0140000

+#define FILEIO_S_IFLNK        0120000

 #define FILEIO_S_IFREG        0100000
+#define FILEIO_S_IFBLK         060000

 #define FILEIO_S_IFDIR         040000
 #define FILEIO_S_IFCHR         020000
+#define FILEIO_S_IFIFO         010000

+#define FILEIO_S_IFMT         0170000

 #define FILEIO_S_IRUSR           0400
 #define FILEIO_S_IWUSR           0200
 #define FILEIO_S_IXUSR           0100
@@ -53,9 +67,8 @@ 

 #define FILEIO_S_IWOTH             02
 #define FILEIO_S_IXOTH             01
 #define FILEIO_S_IRWXO             07
-#define FILEIO_S_SUPPORTED         (FILEIO_S_IFREG|FILEIO_S_IFDIR|  \

-				    FILEIO_S_IRWXU|FILEIO_S_IRWXG|  \

-                                    FILEIO_S_IRWXO)

+#define FILEIO_S_SUPPORTED         (FILEIO_S_IFMT | FILEIO_S_IRWXU| \

+				 FILEIO_S_IRWXG | FILEIO_S_IRWXO)

 
 /* lseek(2) flags */
 #define FILEIO_SEEK_SET             0