Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:52135
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain nebm.ist.utl.pt from 193.136.128.21 cause and error)
Content-Type: multipart/mixed; boundary=----------Y2ZRNxaflvMuGew3hRWYQZ
To: internals@lists.php.net
References: <BANLkTimbSibmdXx_MSNqpUX2Xyke17K94A@mail.gmail.com>
 <op.vuq599voidpuyk@cataphract>
 <BANLkTiks16OT1acO1M2D61o5kybYy-B1Qg@mail.gmail.com>
 <BANLkTik1Os+3HJUoKK0JV0RgjZE=V4s-dQ@mail.gmail.com>
 <BANLkTimYxGaTvn=wy2OdQ6t39SMtjK6fmA@mail.gmail.com>
 <BANLkTinrStuv55X4SH3HTCByzs+VjjnDRg@mail.gmail.com>
Date: Sun, 08 May 2011 03:21:02 +0100
MIME-Version: 1.0
Organization: =?utf-8?Q?N=C3=BAcleo_de_Eng=2E_Biom=C3=A9di?=
 =?utf-8?Q?ca_do_IST?=
Message-ID: <op.vu44tcu3idpuyk@cataphract-old.dulce.lo.geleia.net>
In-Reply-To: <BANLkTinrStuv55X4SH3HTCByzs+VjjnDRg@mail.gmail.com>
User-Agent: Opera Mail/11.10 (Win32)
Subject: Re: [PHP-DEV] adding low level file handling stuff
From: glopes@nebm.ist.utl.pt ("Gustavo Lopes")

------------Y2ZRNxaflvMuGew3hRWYQZ
Content-Type: text/plain; charset=utf-8; format=flowed; delsp=yes
Content-Transfer-Encoding: 7bit

On Sat, 07 May 2011 00:48:46 +0100, Ferenc Kovacs <info@tyrael.hu> wrote:

> On Sat, May 7, 2011 at 1:40 AM, Ferenc Kovacs <info@tyrael.hu> wrote:
>> On Thu, May 5, 2011 at 2:30 AM, Ferenc Kovacs <info@tyrael.hu> wrote:
>>> On Sat, Apr 30, 2011 at 5:23 PM, Ferenc Kovacs <info@tyrael.hu> wrote:
>>>>>
>>>>> I actually did consider adding support for an extended form of  
>>>>> php://fd where one would specify the desired file descriptor:  
>>>>> php://fd/<orig fd>/<new fd>. It would call dup2 instead of dup.
>>>>>
>>>>> However, I got into some trouble on shutdown because this could cause
>>>>> stdout to be closed ahead of time and then the output subsystem  
>>>>> would cause either a segfault or a memory leak (can't recall). I  
>>>>> didn't spend more than 20 minutes on this as it was not the problem  
>>>>> I was trying to solve, so there's probably an easy solution. If you  
>>>>> want to work on this, extending php://fd would likely be a good  
>>>>> place.
>>>>>
>>>> adding dup2 support for the php://fd stream would be better than the
>>>> current situation, but I can't see why are we trying to force  
>>>> everything into that.
>>>> I mean everything on the http://www.php.net/manual/en/wrappers.php  
>>>> can be
>>>> achived through a function also.
>>>> there are some case when you can save a couple of lines of code with  
>>>> the wrappers and with http://www.php.net/manual/en/context.http.php  
>>>> but there is no method for opening FDs or the dup2 that you mentioned.
>>>> I think that it would be more consistent with the rest of the  
>>>> language, and one would more likely to find fdopen than  
>>>> fopen('php://fd/1'); in the documentation.

I just don't see the point of duplicating functionality, especially when  
there's already a closely related wrapper.

It's not true that everything in http://php.net/wrappers can can be done  
in another fashion. In fact, for the php:// wrappers, which is what we're  
talking about, ONLY ONE (php://filter) duplicates functionality that can  
be achieved with functions.

>>>> closing the stdout without reopening also caused problems for me in  
>>>> case of an error happens, but this shouldn't happen if you properly  
>>>> reopen it, so I would be interested what exactly happened there.

Yes, this causes a memory leak. Maybe that was the problem I found. Duping  
the file descriptor after the call to dup2 would solve this, which I think  
is one more reason not to expose these low-level functions directly and  
provide a safer alternative (more below).

>>>
>>> I put together the fileno, fdopen, dup2, fclose stuff as a pecl
>>> extension(called fildes), the sourcecode available here:
>>> https://github.com/Tyrael/php-fildes
>>> [...]
>> Could somebody review the source? as I mentioned in my previous email
>> I had some problem here:
>> https://github.com/Tyrael/php-fildes/blob/master/fildes.c#L211
>>
>> if I define the second argument as int, the first argument will be  
>> always 0.
>> I have no idea whats going there.
>>

See http://lxr.php.net/xref/PHP_TRUNK/README.PARAMETER_PARSING_API#72

> and I forgot to ask the most important thing:
> what do you think about the feature itself?
> I would really like to have it in the core, but I think it would be
> better to have this as functions, than adding more stream magic.
> I'm not sure where would this functions fit the best.
> 1, it could be added to the filesystem functions (we already have some
> which are only supported on some systems)
> 2, it could be added to the posix functions (would be logical)
> 3, it could be added as stream stuff (Gustavo supports this imo)
> 4, it could be added to the dio pecl extension (could fit in there,
> but it's not in the core, and the package doesn't seem much alive)
> 5, it could be added to pecl as a different extension (bad idea, imo)
>

PHP tries to abstract away these kind of details; I don't think direct  
access to low level I/O functions is appropriate in the core. The dio pecl  
extension would seem more appropriate. I don't know how alive it is, but  
if I recall correctly, it got a new maintainer last year.

The POSIX extension would also seem to make sense, but it seems more  
focused on another class of functions and is not available on Windows.

That said, if some useful use case is not available (or is available, but  
it's awkward) in PHP, we should try to fix it. If I understand correctly,  
what compels you is being able to replace stdin, stdout and stderr.
I would tend to support #3; for reference here goes the change to php://fd  
that would be necessary. The problem is, as far as I know, there's no way  
to retrieve the file descriptor associated a stream, so we'd still have to  
rely on opening order.

One possible solution would be to add this information to the wrapper data  
of the "plain files" and in systems other than Windows, to other relevant  
stream types (e.g. sockets). I think your solution of calling stream_cast  
is a good option because in Windows the socket streams return socket  
handles -- file descriptors are an abstraction in the C lib, not really  
part of the windows API -- and because in some stream implementations that  
call has important side effects.

Another option would option would be to unburden the user from dealing  
with file descriptors completely and provide a function to replace the  
standard file descriptors with a stream.
-- 
Gustavo Lopes
------------Y2ZRNxaflvMuGew3hRWYQZ
Content-Disposition: attachment; filename=php_fd_ext.txt
Content-Type: text/plain; name==??Q?php=5Ffd=5Fext.txt?=
Content-Transfer-Encoding: 7bit

Index: php_fopen_wrapper.c
===================================================================
--- php_fopen_wrapper.c	(revision 310834)
+++ php_fopen_wrapper.c	(working copy)
@@ -260,16 +260,30 @@
 	} else if (!strncasecmp(path, "fd/", 3)) {
 		char	   *start,
 				   *end;
-		long	   fildes_ori;
+		long	   fildes_ori,
+				   fildes_des;
 		int		   dtablesize;
+		int		   has_des = 0;
 
 		start = &path[3];
 		fildes_ori = strtol(start, &end, 10);
-		if (end == start || *end != '\0') {
+		if (end == start || (*end != '\0' && *end != '/')) {
 			php_stream_wrapper_log_error(wrapper, options TSRMLS_CC,
-				"php://fd/ stream must be specified in the form php://fd/<orig fd>");
+				"php://fd/ stream must be specified in the form "
+				"php://fd/<orig fd>[/<target fd>]");
 			return NULL;
 		}
+		if (*end == '/') {
+			has_des = 1;
+			start = end + 1;
+			fildes_des = strtol(start, &end, 10);
+			if (end == start || *end != '\0') {
+				php_stream_wrapper_log_error(wrapper, options TSRMLS_CC,
+					"php://fd/ stream must be specified in the form "
+					"php://fd/<orig fd>[/<target fd>]");
+				return NULL;
+			}
+		}
 
 #if HAVE_UNISTD_H
 		dtablesize = getdtablesize();
@@ -277,13 +291,25 @@
 		dtablesize = INT_MAX;
 #endif
 
-		if (fildes_ori < 0 || fildes_ori >= dtablesize) {
+		if (fildes_ori < 0 || fildes_ori >= dtablesize ||
+				(has_des && (fildes_des < 0 || fildes_des >= dtablesize ))) {
 			php_stream_wrapper_log_error(wrapper, options TSRMLS_CC,
 				"The file descriptors must be non-negative numbers smaller than %d", dtablesize);
 			return NULL;
 		}
 		
-		fd = dup(fildes_ori);
+		if (has_des) {
+			fd = dup2((int)fildes_ori, (int)fildes_des);
+			if (fd == -1) {
+				php_stream_wrapper_log_error(wrapper, options TSRMLS_CC,
+					"Error dupping %ld to %ld: [%d]: %s", fildes_ori, fildes_des,
+					errno, strerror(errno));
+				return NULL;
+			}
+			fildes_ori = fildes_des; /* we're now dupping it */
+		}
+
+		fd = dup((int)fildes_ori);
 		if (fd == -1) {
 			php_stream_wrapper_log_error(wrapper, options TSRMLS_CC,
 				"Error duping file descriptor %ld; possibly it doesn't exist: "

------------Y2ZRNxaflvMuGew3hRWYQZ--