Re: Resource logging PHP extension

Hi, On Fri, 2010-09-17 at 18:25 +0200, Bostjan Skufca wrote: > > 1) License headers missing > Indeed, fixed by adding PHP 3.01 license to all .c and .h files. Great! > > > 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. See the comment in main/strlcpy.c: /* * Copy src to string dst of size siz. At most siz-1 characters * will be copied. Always NUL terminates (unless siz == 0). * Returns strlen(src); if retval >= siz, truncation occurred. */ The second sentence should answer your question :-) If not: There's a paper about it by Todd C. Miller and Theo de Raadt: http://www.usenix.org/events/usenix99/millert.html > 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... I assume it can't really work in threaded mode at all, as the kernel can't separate the resource usage from different threads, it only sees a big process using lots of memory. I think the best thing would be to prevent building using some config.m4 magic, which I can't come up with. The second best might be something like #ifdef ZTS #error "Not in TSRM mode!" #endif in the .c file ... > 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. You might test that at least some memory was used or something along the lines maybe. A trivial test might be enough to at least validate the build worked. > I have nothing against it. > > > In general the extension might be acceptable for PECL. > > This is encouraging :) Please fill in the account registration forms (don't have a URLat hand, but should be linked from pecl.php.net) :-) johannes