Re: Resource logging PHP extension

This thing got delayet a bit, but anyway, here I am now. First of all, thank you for your extended comments, Johannes. The answers (and questions:) are below. b. 2010/9/7 Johannes Schlüter <johannes@php.net> > Hi, > > On Tue, 2010-09-07 at 02:10 +0200, Bostjan Skufca wrote: > > Hi all! > > > > I have created a PHP extension for system resources usage logging. I find > it > > very useful for virtual hosting environment (gives insight in CPU/time > usage > > per vhost etc). > > > > I would like to share it with the world by including it in PECL. It is > > tested with PHP versions 5.2.14 and 5.5.3. I use it in production without > > any problems whatsoever. The code can be found here: > > http://sourceforge.net/projects/rsclog/ > > I had a quick look over the code, some comments: > > 1) License headers missing > > By that it is not clear which license applies for every file. Please add > them, as BSD/PHP License work file based a single Copying file is not > enough. > Indeed, fixed by adding PHP 3.01 license to all .c and .h files. 2) strncpy > > You are using strncpy, we prefer usage of PHP's strlcpy which is safer, > like adding checks to make sure the string is really NULL terminated. > Didn't know about this one, fixed. BUT: does it copy one character less? I had to add +1 to its third argument because it resulted in "/proc/self/sta" error. > 3) /proc usage > > The README says this would work on Unix, I can find > neither /proc/self/stat not /proc/$pid/stat on Solaris (but can > find /proc/$pid/status, no idea if that helps) so that assumption in the > README should be fixed, might also be good to add a test in the > config.m4 for this. > Fixed README to annotate it as Linux-only extension. See 5) for more. 4) C99/C++ comments > > Don't use // style comments in PHP, but always /* */ or comment out > using #ifdef. > Stale entries, removed. 5) thread safety > > The extension depends on true globals to transport data, please look > into TSRM to add isolation (... or at least prevent compiling this > extension in thread-safe mode and document as such) > I realised that with current implementation, this is not only not-thread-safe, but thread-incompatible. But I am a bit in the dark here on how to do it. I would like this extension to build only on linux systems and only when thread safety is not enabled. Can you point me to some location where this is already done or hint me on how to do it? I would greatly appreciate this... > 6) Tests > > Tests hould be written using PHP's phpt format so one can run them via > make test, see http://qa.php.net/write-test.php > Those were some "tiny" scripts to help me with development, not the tests intended to be run by "make test". Honestly I would like to create some tests but as this extension's output is too syslog config dependent I can't think of any particular (and sufficiently elegant) way to make this work. > > The "Requirements" website (http://pecl.php.net/account-request.php) > states > > I should discuss aforementioned inclusion on this list first, so here I > am. > > :) > > What is the next step? > > Would you be willing to move over to PHP's svn server or do you want to > keep on sourceforge's git? > I have nothing against it. > In general the extension might be acceptable for PECL. > This is encouraging :) > johannes > >