fdclose(3) patch

[p5pRT]

Migrated from rt.perl.org#126847 (status was 'resolved')

Searchable as RT126847$

[p5pRT]

From oshogbo@FreeBSD.org

Hello,

Some time ago we add to FreeBSD a function called fdclose(3).
Idea of this function is to free FILE structure but leave/return
underlining descriptor. This is *cleaner* way instead messing in FILE structure.

fdclose(3) is available in FreeBSD 11 [2] and FreeBSD 10/stable [3].
Patch is available at [1].

[1] https://people.freebsd.org/~oshogbo/perl-fdclose.patch
[2]
https://svnweb.freebsd.org/base/head/lib/libc/stdio/fclose.c?revision=285140&view=markup
[3]
https://svnweb.freebsd.org/base/stable/10/lib/libc/stdio/fclose.c?revision=291602&view=markup

Thanks,
--
Mariusz Zaborski
oshogbo//vx | http​://oshogbo.vexillium.org
FreeBSD commiter | https://freebsd.org
Software developer | http​://wheelsystems.com
If it's not broken, let's fix it till it is!!1

[p5pRT]

From @jkeenan

On Tue Dec 08 12​:05​:27 2015, oshogbo@​FreeBSD.org wrote​:

Hello,

Some time ago we add to FreeBSD a function called fdclose(3).
Idea of this function is to free FILE structure but leave/return
underlining descriptor. This is *cleaner* way instead messing in FILE
structure.

fdclose(3) is available in FreeBSD 11 [2] and FreeBSD 10/stable [3].
Patch is available at [1].

[1] https://people.freebsd.org/~oshogbo/perl-fdclose.patch
[2]
https://svnweb.freebsd.org/base/head/lib/libc/stdio/fclose.c?revision=285140&view=markup
[3]
https://svnweb.freebsd.org/base/stable/10/lib/libc/stdio/fclose.c?revision=291602&view=markup

Thanks,

I have forked a branch from blead and applied your patch to it. It is smoke-testing in branch smoke-me/jkeenan/126847-freebsd-fdclose.

However, it occurs to me that Perl 5 probably has to prepare its source code to run on versions of FreeBSD older than 10/stable (even though FreeBSD itself does not). We'll need guidance from P5P about this.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @tonycoz

On Tue Dec 08 16​:10​:44 2015, jkeenan wrote​:

However, it occurs to me that Perl 5 probably has to prepare its
source code to run on versions of FreeBSD older than 10/stable (even
though FreeBSD itself does not). We'll need guidance from P5P about
this.

Yes, we support older revisions of FreeBSD, so the patch as it stands is unsuitable.

If it's at all likely this will make it into other BSDs (or other unrelated libcs), we'd probably want to detect it in Configure, but I don't see any pertinent google matches for fdclose with the other BSDs.

I think for now we'd need to detect it based on the OS release in hints/freebsd.sh.

Tony

[p5pRT]

From @jkeenan

On Tue Dec 08 16​:40​:29 2015, tonyc wrote​:

On Tue Dec 08 16​:10​:44 2015, jkeenan wrote​:

However, it occurs to me that Perl 5 probably has to prepare its
source code to run on versions of FreeBSD older than 10/stable (even
though FreeBSD itself does not). We'll need guidance from P5P about
this.

Yes, we support older revisions of FreeBSD, so the patch as it stands
is unsuitable.

If it's at all likely this will make it into other BSDs (or other
unrelated libcs), we'd probably want to detect it in Configure, but I
don't see any pertinent google matches for fdclose with the other
BSDs.

I think for now we'd need to detect it based on the OS release in
hints/freebsd.sh.

Tony

One other concern​: At the present time, we have no one filing smoke-test reports of Perl blead on FreeBSD. See​: http​://perl.develop-help.com/

--
James E Keenan (jkeenan@​cpan.org)

[p5pRT]

From oshogbo@FreeBSD.org

On Tue, Dec 08, 2015 at 04​:40​:30PM -0800, Tony Cook via RT wrote​:

On Tue Dec 08 16​:10​:44 2015, jkeenan wrote​:

However, it occurs to me that Perl 5 probably has to prepare its
source code to run on versions of FreeBSD older than 10/stable (even
though FreeBSD itself does not). We'll need guidance from P5P about
this.

Yes, we support older revisions of FreeBSD, so the patch as it stands is unsuitable.

If it's at all likely this will make it into other BSDs (or other unrelated libcs), we'd probably want to detect it in Configure, but I don't see any pertinent google matches for fdclose with the other BSDs.

I think for now we'd need to detect it based on the OS release in hints/freebsd.sh.

Tony

Thanks for quick response and thank you for good points.

I was thinking about bumping a FreeBSD_version in stable and we could detected
it in code something like​:
#if (__FreeBSD_verion > 1002503)
  fdclose...
#else
  ...
#endif

I'm not sure how other BSDs will or will not want to implement this function.
If you prefer detecting it in the configure its sound good for me.

--
Mariusz Zaborski
oshogbo//vx | http​://oshogbo.vexillium.org
FreeBSD commiter | https://freebsd.org
Software developer | http​://wheelsystems.com
If it's not broken, let's fix it till it is!!1

[p5pRT]

From oshogbo@FreeBSD.org

On Tue, Dec 08, 2015 at 05​:34​:49PM -0800, James E Keenan via RT wrote​:

On Tue Dec 08 16​:40​:29 2015, tonyc wrote​:

On Tue Dec 08 16​:10​:44 2015, jkeenan wrote​:

However, it occurs to me that Perl 5 probably has to prepare its
source code to run on versions of FreeBSD older than 10/stable (even
though FreeBSD itself does not). We'll need guidance from P5P about
this.

Yes, we support older revisions of FreeBSD, so the patch as it stands
is unsuitable.

If it's at all likely this will make it into other BSDs (or other
unrelated libcs), we'd probably want to detect it in Configure, but I
don't see any pertinent google matches for fdclose with the other
BSDs.

I think for now we'd need to detect it based on the OS release in
hints/freebsd.sh.

Tony

One other concern​: At the present time, we have no one filing smoke-test reports of Perl blead on FreeBSD. See​: http​://perl.develop-help.com/

Thanks for quick response and thank you for good points.

I was thinking about bumping a FreeBSD_version in stable and we could detected
it in code something like​:
#if (__FreeBSD_verion > 1002503)
fdclose...
#else
...
#endif

I'm not sure how other BSDs will or will not want to implement this function.
If you prefer detecting it in the configure its sound good for me.

--
Mariusz Zaborski
oshogbo//vx | http​://oshogbo.vexillium.org
FreeBSD commiter | https://freebsd.org
Software developer | http​://wheelsystems.com
If it's not broken, let's fix it till it is!!1

[p5pRT]

From oshogbo@FreeBSD.org

Thanks for quick response and thank you for good points.

On Tue, Dec 08, 2015 at 04​:40​:30PM -0800, Tony Cook via RT wrote​:

On Tue Dec 08 16​:10​:44 2015, jkeenan wrote​:

However, it occurs to me that Perl 5 probably has to prepare its
source code to run on versions of FreeBSD older than 10/stable (even
though FreeBSD itself does not). We'll need guidance from P5P about
this.

Yes, we support older revisions of FreeBSD, so the patch as it stands is unsuitable.

If it's at all likely this will make it into other BSDs (or other unrelated libcs), we'd probably want to detect it in Configure, but I don't see any pertinent google matches for fdclose with the other BSDs.

I think for now we'd need to detect it based on the OS release in hints/freebsd.sh.
I was thinking about bumping a FreeBSD_version in stable and we could detected
it in code something like​:
#if (__FreeBSD_verion > 1002503)
  fdclose...
#else
  ...
#endif

I'm not sure how other BSDs will or will not want to implement this function.
If you prefer detecting it in the configure its sound good for me.

Thanks,
--
Mariusz Zaborski
oshogbo//vx | http​://oshogbo.vexillium.org
FreeBSD commiter | https://freebsd.org
Software developer | http​://wheelsystems.com
If it's not broken, let's fix it till it is!!1

[p5pRT]

From oshogbo@FreeBSD.org

Patch in attachment.
What do you thinks?

On Tue, Dec 08, 2015 at 05​:34​:49PM -0800, James E Keenan via RT wrote​:

On Tue Dec 08 16​:40​:29 2015, tonyc wrote​:

On Tue Dec 08 16​:10​:44 2015, jkeenan wrote​:

However, it occurs to me that Perl 5 probably has to prepare its
source code to run on versions of FreeBSD older than 10/stable (even
though FreeBSD itself does not). We'll need guidance from P5P about
this.

Yes, we support older revisions of FreeBSD, so the patch as it stands
is unsuitable.

If it's at all likely this will make it into other BSDs (or other
unrelated libcs), we'd probably want to detect it in Configure, but I
don't see any pertinent google matches for fdclose with the other
BSDs.

I think for now we'd need to detect it based on the OS release in
hints/freebsd.sh.

Tony

One other concern​: At the present time, we have no one filing smoke-test reports of Perl blead on FreeBSD. See​: http​://perl.develop-help.com/

--
James E Keenan (jkeenan@​cpan.org)

--
Mariusz Zaborski
oshogbo//vx | http​://oshogbo.vexillium.org
FreeBSD commiter | https://freebsd.org
Software developer | http​://wheelsystems.com
If it's not broken, let's fix it till it is!!1

[p5pRT]

From oshogbo@FreeBSD.org

fdclose.patch

diff --git a/perlio.c b/perlio.c
index 343c62e..97fcb1a 100644
--- a/perlio.c
+++ b/perlio.c
@@ -58,6 +58,10 @@ int mkstemp(char*);
 #include <rms.h>
 #endif
 
+#if defined(__FreeBSD__)
+#include <sys/param.h>
+#endif
+
 #define PerlIO_lockcnt(f) (((PerlIOl*)(f))->head->flags)
 
 /* Call the callback or PerlIOBase, and return failure. */
@@ -3152,12 +3156,12 @@ PerlIOStdio_invalidate_fileno(pTHX_ FILE *f)
     f->_file = -1;
     return 1;
 #  elif defined(__FreeBSD__)
-    /* There may be a better way on FreeBSD:
-        - we could insert a dummy func in the _close function entry
-	f->_close = (int (*)(void *)) dummy_close;
-     */
+#  if (__FreeBSD_version > 1002503 && __FreeBSD_version < 1100000) || __FreeBSD_version > 1100091
+    return fdclose(f, NULL) == 0 ? 1 : 0;
+#  else
     f->_file = -1;
     return 1;
+#  endif
 #  elif defined(__OpenBSD__)
     /* There may be a better way on OpenBSD:
         - we could insert a dummy func in the _close function entry

[p5pRT]

From @doughera88

On Tue, Dec 15, 2015 at 06​:01​:15PM +0100, Mariusz Zaborski wrote​:

Patch in attachment.
What do you thinks?

Thank you for the patch. I have attached two suggested patches which
ought to have the equivalent functionality, but might be a bit
more general.

The first simply uses fdclose() if it's available as the first line of
attack in PerlIOStdio_invalidate_fileno. I did not make it specific to
FreeBSD in case other OSes also adopt this function. The second (much
longer) adds Configure support for the HAS_FDCLOSE symbol. This is
currently smoking in the smoke-me/andyd/fdclose branch.

I did note that a standard run of 'make test' never seems to call this
function, so I don't know how much it is actually used.

--
  Andy Dougherty doughera@​lafayette.edu
  Dept. of Physics
  Lafayette College, Easton PA 18042

[p5pRT]

From @doughera88

0001-PATCH-Re-perl-126847-fdclose-3-patch.patch

From 3f58143a4370507d7ceccc8d8c378559b1e5aee8 Mon Sep 17 00:00:00 2001
From: Andy Dougherty <doughera@lafayette.edu>
Date: Tue, 29 Dec 2015 22:47:42 -0500
Subject: [PATCH 1/2] PATCH: Re: [perl #126847] fdclose(3) patch

This patch uses the fdclose() function from FreeBSD if it
is available.  It is based on the original patch supplied
by Mariusz Zaborski <oshogbo@FreeBSD.org> in the RT ticket.

The next patch will add Configure support for HAS_FDCLOSE.
---
 perlio.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/perlio.c b/perlio.c
index 343c62e..69f3755 100644
--- a/perlio.c
+++ b/perlio.c
@@ -3126,7 +3126,9 @@ PerlIOStdio_invalidate_fileno(pTHX_ FILE *f)
     /* XXX this could use PerlIO_canset_fileno() and
      * PerlIO_set_fileno() support from Configure
      */
-#  if defined(__UCLIBC__)
+#  if defined(HAS_FDCLOSE)
+    return fdclose(f, NULL) == 0 ? 1 : 0;
+#  elif defined(__UCLIBC__)
     /* uClibc must come before glibc because it defines __GLIBC__ as well. */
     f->__filedes = -1;
     return 1;
-- 
2.1.4

[p5pRT]

From @doughera88

0002-Add-Configure-support-for-fdclose-for-perl-126847.patch

From c62f49e3141bbdb4391c19e87b36ff0247151208 Mon Sep 17 00:00:00 2001
From: Andy Dougherty <doughera@lafayette.edu>
Date: Tue, 29 Dec 2015 22:58:51 -0500
Subject: [PATCH 2/2] Add Configure support for fdclose() for [perl #126847].

This patch also adjusts the generated files suggested by
Porting/checkcfgvar.pl.
---
 Configure                 |  6 ++++++
 Cross/config.sh-arm-linux |  1 +
 NetWare/config.wc         |  1 +
 Porting/config.sh         |  1 +
 config_h.SH               |  7 +++++++
 configure.com             |  1 +
 plan9/config_sh.sample    |  1 +
 symbian/config.sh         |  1 +
 uconfig.h                 | 11 +++++++++--
 uconfig.sh                |  1 +
 uconfig64.sh              |  1 +
 win32/config.ce           |  1 +
 win32/config.gc           |  1 +
 win32/config.vc           |  1 +
 14 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/Configure b/Configure
index 0e71b4b..54ef4c6 100755
--- a/Configure
+++ b/Configure
@@ -475,6 +475,7 @@ d_fcntl_can_lock=''
 d_fd_macros=''
 d_fd_set=''
 d_fds_bits=''
+d_fdclose=''
 d_fdim=''
 d_fegetround=''
 d_fgetpos=''
@@ -13991,6 +13992,10 @@ EOM
 fi
 $rm_try
 
+: see if fdclose exists
+set fdclose d_fdclose
+eval $inlibc
+
 : see if fdim exists
 set fdim d_fdim
 eval $inlibc
@@ -24213,6 +24218,7 @@ d_fcntl='$d_fcntl'
 d_fcntl_can_lock='$d_fcntl_can_lock'
 d_fd_macros='$d_fd_macros'
 d_fd_set='$d_fd_set'
+d_fdclose='$d_fdclose'
 d_fdim='$d_fdim'
 d_fds_bits='$d_fds_bits'
 d_fegetround='$d_fegetround'
diff --git a/Cross/config.sh-arm-linux b/Cross/config.sh-arm-linux
index 9e3ce16..045ba71 100644
--- a/Cross/config.sh-arm-linux
+++ b/Cross/config.sh-arm-linux
@@ -199,6 +199,7 @@ d_fcntl='define'
 d_fcntl_can_lock='define'
 d_fd_macros='define'
 d_fd_set='define'
+d_fdclose='undef'
 d_fdim='undef'
 d_fds_bits='undef'
 d_fegetround='define'
diff --git a/NetWare/config.wc b/NetWare/config.wc
index e70b4df..d414e98 100644
--- a/NetWare/config.wc
+++ b/NetWare/config.wc
@@ -187,6 +187,7 @@ d_fcntl='undef'
 d_fcntl_can_lock='undef'
 d_fd_macros='define'
 d_fd_set='define'
+d_fdclose='undef'
 d_fdim='undef'
 d_fds_bits='define'
 d_fegetround='undef'
diff --git a/Porting/config.sh b/Porting/config.sh
index ec37c3e..dc1d8ea 100644
--- a/Porting/config.sh
+++ b/Porting/config.sh
@@ -208,6 +208,7 @@ d_fcntl='define'
 d_fcntl_can_lock='define'
 d_fd_macros='define'
 d_fd_set='define'
+d_fdclose='undef'
 d_fdim='undef'
 d_fds_bits='undef'
 d_fegetround='undef'
diff --git a/config_h.SH b/config_h.SH
index 01ac23d..e083524 100755
--- a/config_h.SH
+++ b/config_h.SH
@@ -3561,6 +3561,13 @@ sed <<!GROK!THIS! >$CONFIG_H -e 's!^#undef\(.*/\)\*!/\*#define\1 \*!' -e 's!^#un
  */
 #$d_fcntl_can_lock FCNTL_CAN_LOCK		/**/
 
+/* HAS_FDCLOSE:
+ *	This symbol, if defined, indicates that the fdclose routine is
+ *	available to free a FILE structure without closing the underlying
+ *	file descriptor.  This function appeared in FreeBSD 10.2.
+ */
+#$d_fdclose HAS_FDCLOSE		/**/
+
 /* HAS_FDIM:
  *	This symbol, if defined, indicates that the fdim routine is
  *	available to do the positive difference function.
diff --git a/configure.com b/configure.com
index 6fdffe0..2d674c5 100644
--- a/configure.com
+++ b/configure.com
@@ -6003,6 +6003,7 @@ $ WC "d_fcntl='" + d_fcntl + "'"
 $ WC "d_fcntl_can_lock='" + d_fcntl_can_lock + "'"
 $ WC "d_fd_set='" + d_fd_set + "'"
 $ WC "d_fd_macros='define'"
+$ WC "d_fdclose='undef'"
 $ WC "d_fdim='" + d_fdim + "'"
 $ WC "d_fds_bits='define'"
 $ WC "d_fegetround='undef'"
diff --git a/plan9/config_sh.sample b/plan9/config_sh.sample
index de1d872..17cefda 100644
--- a/plan9/config_sh.sample
+++ b/plan9/config_sh.sample
@@ -199,6 +199,7 @@ d_fcntl='define'
 d_fcntl_can_lock='undef'
 d_fd_macros='undef'
 d_fd_set='undef'
+d_fdclose='undef'
 d_fdim='undef'
 d_fds_bits='undef'
 d_fegetround='undef'
diff --git a/symbian/config.sh b/symbian/config.sh
index a5aa477..c6466c9 100644
--- a/symbian/config.sh
+++ b/symbian/config.sh
@@ -143,6 +143,7 @@ d_fcntl='undef'
 d_fcntl_can_lock='undef'
 d_fd_macros='undef'
 d_fd_set='undef'
+d_fdclose='undef'
 d_fdim='undef'
 d_fds_bits='undef'
 d_fegetround='undef'
diff --git a/uconfig.h b/uconfig.h
index af65ca7..079c73f 100644
--- a/uconfig.h
+++ b/uconfig.h
@@ -3526,6 +3526,13 @@
  */
 /*#define FCNTL_CAN_LOCK		/ **/
 
+/* HAS_FDCLOSE:
+ *	This symbol, if defined, indicates that the fdclose routine is
+ *	available to free a FILE structure without closing the underlying
+ *	file descriptor.  This function appeared in FreeBSD 10.2.
+ */
+/*#define HAS_FDCLOSE		/ **/
+
 /* HAS_FDIM:
  *	This symbol, if defined, indicates that the fdim routine is
  *	available to do the positive difference function.
@@ -5175,6 +5182,6 @@
 #endif
 
 /* Generated from:
- * 1fbdd1f584710d990cbc1b624770986e12ad6e3eac21c9f3851e6a0ad5a7fbce config_h.SH
- * 0ce9d24f6ed83c533882929bc7c0138fe345656c4b7070aad99bb103dbf3790a uconfig.sh
+ * 056bddc3ae72075045127f87d3418b2caa44b81bdac54fa0a1a3cf11c8fbdd4a config_h.SH
+ * a44240b1f83708d59d68e3c6d4d37d0c55ea9a2bab6e0413c17236c9000df18d uconfig.sh
  * ex: set ro: */
diff --git a/uconfig.sh b/uconfig.sh
index bd889e3..c705090 100644
--- a/uconfig.sh
+++ b/uconfig.sh
@@ -137,6 +137,7 @@ d_fcntl='undef'
 d_fcntl_can_lock='undef'
 d_fd_macros='undef'
 d_fd_set='undef'
+d_fdclose='undef'
 d_fdim='undef'
 d_fds_bits='undef'
 d_fegetround='undef'
diff --git a/uconfig64.sh b/uconfig64.sh
index ec09c1e..8835d61 100644
--- a/uconfig64.sh
+++ b/uconfig64.sh
@@ -138,6 +138,7 @@ d_fcntl='undef'
 d_fcntl_can_lock='undef'
 d_fd_macros='undef'
 d_fd_set='undef'
+d_fdclose='undef'
 d_fdim='undef'
 d_fds_bits='undef'
 d_fegetround='undef'
diff --git a/win32/config.ce b/win32/config.ce
index 3c10d77..2c978a5 100644
--- a/win32/config.ce
+++ b/win32/config.ce
@@ -185,6 +185,7 @@ d_fcntl='undef'
 d_fcntl_can_lock='undef'
 d_fd_macros='define'
 d_fd_set='define'
+d_fdclose='undef'
 d_fdim='undef'
 d_fds_bits='define'
 d_fegetround='undef'
diff --git a/win32/config.gc b/win32/config.gc
index e0eb238..7b37e54 100644
--- a/win32/config.gc
+++ b/win32/config.gc
@@ -186,6 +186,7 @@ d_fcntl='undef'
 d_fcntl_can_lock='undef'
 d_fd_macros='define'
 d_fd_set='define'
+d_fdclose='undef'
 d_fdim='undef'
 d_fds_bits='define'
 d_fegetround='undef'
diff --git a/win32/config.vc b/win32/config.vc
index b4efd32..8bf7fba 100644
--- a/win32/config.vc
+++ b/win32/config.vc
@@ -186,6 +186,7 @@ d_fcntl='undef'
 d_fcntl_can_lock='undef'
 d_fd_macros='define'
 d_fd_set='define'
+d_fdclose='undef'
 d_fdim='undef'
 d_fds_bits='define'
 d_fegetround='undef'
-- 
2.1.4

[p5pRT]

From oshogbo@FreeBSD.org

On Thu, Dec 31, 2015 at 11​:44​:10AM -0500, Andy Dougherty wrote​:

On Tue, Dec 15, 2015 at 06​:01​:15PM +0100, Mariusz Zaborski wrote​:

Patch in attachment.
What do you thinks?

Thank you for the patch. I have attached two suggested patches which
ought to have the equivalent functionality, but might be a bit
more general.

The first simply uses fdclose() if it's available as the first line of
attack in PerlIOStdio_invalidate_fileno. I did not make it specific to
FreeBSD in case other OSes also adopt this function. The second (much
longer) adds Configure support for the HAS_FDCLOSE symbol. This is
currently smoking in the smoke-me/andyd/fdclose branch.

I did note that a standard run of 'make test' never seems to call this
function, so I don't know how much it is actually used.
It's looking great for me!!!
Thanks!

Cheers,
--
Mariusz Zaborski
oshogbo//vx | http​://oshogbo.vexillium.org
FreeBSD commiter | https://freebsd.org
Software developer | http​://wheelsystems.com
If it's not broken, let's fix it till it is!!1

[p5pRT]

From @doughera88

On Sat, Jan 02, 2016 at 03​:52​:48PM +0100, Mariusz Zaborski wrote​:

On Thu, Dec 31, 2015 at 11​:44​:10AM -0500, Andy Dougherty wrote​:

On Tue, Dec 15, 2015 at 06​:01​:15PM +0100, Mariusz Zaborski wrote​:

Patch in attachment.
What do you thinks?

Thank you for the patch. I have attached two suggested patches which
ought to have the equivalent functionality, but might be a bit
more general.

The first simply uses fdclose() if it's available as the first line of
attack in PerlIOStdio_invalidate_fileno. I did not make it specific to
FreeBSD in case other OSes also adopt this function. The second (much
longer) adds Configure support for the HAS_FDCLOSE symbol. This is
currently smoking in the smoke-me/andyd/fdclose branch.

I did note that a standard run of 'make test' never seems to call this
function, so I don't know how much it is actually used.
It's looking great for me!!!

Now pushed as
  8b8c6ab
and
  36b1c89

I have separately pushed the d_fdclose.U metaconfig unit to perl's
metaconfig repository.

Let me know of any problems,

--
  Andy Dougherty doughera@​lafayette.edu