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

 
From gdb-patches-return-148573-listarch-gdb-patches=sources.redhat.com@sourceware.org Fri Jun 29 14:28:09 2018
Return-Path: <gdb-patches-return-148573-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Delivered-To: listarch-gdb-patches@sources.redhat.com
Received: (qmail 57622 invoked by alias); 29 Jun 2018 14:28:09 -0000
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.sourceware.org>
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
Delivered-To: mailing list gdb-patches@sourceware.org
Received: (qmail 57092 invoked by uid 89); 29 Jun 2018 14:28:09 -0000
Authentication-Results: sourceware.org; auth=none
X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=replies
X-HELO: mx1.redhat.com
Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 29 Jun 2018 14:28:07 +0000
Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4])	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))	(No client certificate requested)	by mx1.redhat.com (Postfix) with ESMTPS id 7B7B7406C7A3;	Fri, 29 Jun 2018 14:28:06 +0000 (UTC)
Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4])	by smtp.corp.redhat.com (Postfix) with ESMTP id D6C432026D68;	Fri, 29 Jun 2018 14:28:05 +0000 (UTC)
Subject: Re: [PATCH v3] Allow using special files with File I/O functions
To: Julio Guerra <julio@farjump.io>, "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
References: <20180628192635.44056-1-julio@farjump.io> <0102016447dcf9e9-3989bcd9-1272-4a05-93c5-77823c7a0921-000000@eu-west-1.amazonses.com> <3c1caf7f-f50f-a8d7-43f3-f8fa8eca663d@redhat.com> <79758ca1-2541-9ae6-d793-b367d6094468@farjump.io> <010201644bd94c4b-0d8759a9-5625-4773-a858-6e218d4fc9db-000000@eu-west-1.amazonses.com>
From: Pedro Alves <palves@redhat.com>
Message-ID: <9e53034b-7e28-625e-ad70-fbb53863c7e1@redhat.com>
Date: Fri, 29 Jun 2018 14:28:00 -0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <010201644bd94c4b-0d8759a9-5625-4773-a858-6e218d4fc9db-000000@eu-west-1.amazonses.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
X-SW-Source: 2018-06/txt/msg00716.txt.bz2
Content-length: 1419

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