Re: adding low level file handling stuff

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

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: "