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

["Julio Guerra" <julio@farjump.io>]

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