[PHP-DEV] Andy Wharmby/UK/IBM is out of the office.
I will be out of the office starting 24/12/2007 and will not return until 02/01/2008. I will have no email access during this time. I will respond to your email asap on my return. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: #40581 [Opn-Asn]: Pass Struct type to COM object from PHP
Hi Pierre Sorry been very busy on other things in last few months and so have not had the time to give COM any TLC. I will do my best to find some time in the coming weeks to work on some of the outstanding COM defects and of course when I do so I will look at all the outstanding COM defects; whether OPEN or assigned to Wez. Regards Andy Andy Wharmby IBM United Kingdom Limited Hursley Park, UK Pierre wrote: On 8/16/07, Wez Furlong [EMAIL PROTECTED] wrote: Per previous discussion on internals, there are other more active maintainers for COM right now, and by assigning to me, you're assigning to the wrong person. Andy Wharmby is active. I added him in CC. Andy, can you take a look at all COM/dotnet bugs including those assigned to Wez? Jani assigned to Wet a lot of them yesterday :) If you don't care, that's fine, it's just that assigned is not really the correct disposition for the ticket in that case; better to leave it open so that someone else can clearly see that nothing is happening, and perhaps then fix it themselves. The idea behind assign was to get some love for a given bug. That's not really the right way to do it though. I hope the new issues tracker will let us send mails to a group of maintainers when a new issue is raised. Then they can do the right action (and define who will work on it). For now it is just a big source of noises. Cheers, --Pierre -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] [PHP DEV] CVS account extra karma request
Hi Internals I am working with Zoe Slattery and Raghbansh Kumar developing new PHPT testcases for PHP and I would like to request the necesary karma to to commit new tests. I already have a CVS account (id=wharmby) so just need the necessary extra karma to drop the test cases. Regards Andy Andy Wharmby IBM United Kingdom Limited -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Re: COM extension defects: Patch for defect 35872
Hi Frank Thanks for checking my patch out. I will drop code into HEAD on Monday and then email Ilia for OK to drop into 5.2 Regards Andy Frank M. Kromann wrote: Hi ANdy, Looks good to me. Nice and clean. - Frank Hi Internals, Below are details of a suggested patch for COM extension defect 35872 (http://bugs.php.net/bug.php?id=35872) for review/approval. All comments welcome; good or bad. Regards Andy Andy Wharmby IBM United Kingdom Limited Winchester, England SO21 2JN E-mail: [EMAIL PROTECTED] Using supplied testcase the backtrace is as follows: php6ts_debug.dll!zend_object_store_get_object(_zval_struct * zobject=0x009c19e8, void * * * tsrm_ls=0x009659a8) Line 263 + 0x1b C php6ts_debug.dll!zend_objects_get_address(_zval_struct * zobject=0x009c19e8, void * * * tsrm_ls=0x009659a8) Line 144 + 0xdC php6ts_debug.dll!zend_std_object_get_class(_zval_struct * object=0x009c19e8, void * * * tsrm_ls=0x009659a8) Line 1074 + 0xdC php6ts_debug.dll!zend_get_class_entry(_zval_struct * zobject=0x009c19e8, void * * * tsrm_ls=0x009659a8) Line 239 + 0x13 C php6ts_debug.dll!disp_destructor(php_dispatchex * disp=0x00194460) Line 562 + 0x10C php6ts_debug.dll!dispatch_dtor(_zend_rsrc_list_entry * rsrc=0x009c3230, void * * * tsrm_ls=0x009659a8) Line 60 + 0x9C php6ts_debug.dll!list_entry_destructor(void * ptr=0x009c3230) Line 184 + 0x10C php6ts_debug.dll!zend_hash_apply_deleter(_hashtable * ht=0x00968290, bucket * p=0x009c31d8) Line 836 + 0xfC php6ts_debug.dll!zend_hash_graceful_reverse_destroy(_hashtable * ht=0x00968290) Line 871 + 0xdC php6ts_debug.dll!zend_destroy_rsrc_list(_hashtable * ht=0x00968290, void * * * tsrm_ls=0x009659a8) Line 240 + 0x9C php6ts_debug.dll!zend_deactivate(void * * * tsrm_ls=0x009659a8) Line 1358 + 0x1eC php6ts_debug.dll!php_request_shutdown(void * dummy=0x) Line 1429 + 0x9C php.exe!main(int argc=0x0003, char * * argv=0x009658f8) Line 1306 + 0xaC php.exe!mainCRTStartup() Line 398 + 0x11C We fail in zend_object_store_get_object() after the objects store has been destroyed earlier in request shutdown by zend_objects_store_destroy(). Which is very similar to defect 34617 (http://bugs.php.net/bug.php?id=34617). The reference to the object store is caused by the trace() call in disp_destructor(). This call to trace() should probably be a NOOP in a non-debug build but we still need to fix reported problem so DEBUG builds don't hit the problem. I will raise a separate defect to disable the trace calls in non-DEBUG builds. I have resolved the problem reported under defect 35872 by defining a new boolean flag rshutdown_started in COMG which is set to false by COM RINIT routine and true by the COM RSHUTDOWN routine. This allows the COM code to avoid reference to the object store if invoked during request shutdown. The trace calls themselves have been modified to add the object address to allow entries referring to the same object to be correlated even if the object name is not available. Fixing code in disp_destructor revealed a similar issue in FETCH_DISP macro which is fixed in a same way. The full patch; built against CVS HEAD is attached. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] COM extension defects: Patch for defect 35872
Hi Internals,
Below are details of a suggested patch for COM extension defect
35872 (http://bugs.php.net/bug.php?id=35872) for review/approval.
All comments welcome; good or bad.
Regards
Andy
Andy Wharmby
IBM United Kingdom Limited
Winchester, England SO21 2JN
E-mail: [EMAIL PROTECTED]
Using supplied testcase the backtrace is as follows:
php6ts_debug.dll!zend_object_store_get_object(_zval_struct *
zobject=0x009c19e8, void * * * tsrm_ls=0x009659a8) Line 263 + 0x1bC
php6ts_debug.dll!zend_objects_get_address(_zval_struct *
zobject=0x009c19e8, void * * * tsrm_ls=0x009659a8) Line 144 + 0xdC
php6ts_debug.dll!zend_std_object_get_class(_zval_struct *
object=0x009c19e8, void * * * tsrm_ls=0x009659a8) Line 1074 + 0xdC
php6ts_debug.dll!zend_get_class_entry(_zval_struct *
zobject=0x009c19e8, void * * * tsrm_ls=0x009659a8) Line 239 + 0x13C
php6ts_debug.dll!disp_destructor(php_dispatchex * disp=0x00194460)
Line 562 + 0x10C
php6ts_debug.dll!dispatch_dtor(_zend_rsrc_list_entry *
rsrc=0x009c3230, void * * * tsrm_ls=0x009659a8) Line 60 + 0x9C
php6ts_debug.dll!list_entry_destructor(void * ptr=0x009c3230) Line
184 + 0x10C
php6ts_debug.dll!zend_hash_apply_deleter(_hashtable *
ht=0x00968290, bucket * p=0x009c31d8) Line 836 + 0xfC
php6ts_debug.dll!zend_hash_graceful_reverse_destroy(_hashtable *
ht=0x00968290) Line 871 + 0xdC
php6ts_debug.dll!zend_destroy_rsrc_list(_hashtable * ht=0x00968290,
void * * * tsrm_ls=0x009659a8) Line 240 + 0x9C
php6ts_debug.dll!zend_deactivate(void * * * tsrm_ls=0x009659a8)
Line 1358 + 0x1eC
php6ts_debug.dll!php_request_shutdown(void * dummy=0x)
Line 1429 + 0x9C
php.exe!main(int argc=0x0003, char * * argv=0x009658f8) Line
1306 + 0xaC
php.exe!mainCRTStartup() Line 398 + 0x11C
We fail in zend_object_store_get_object() after the objects store has
been destroyed earlier in request shutdown by zend_objects_store_destroy().
Which is very similar to defect 34617
(http://bugs.php.net/bug.php?id=34617).
The reference to the object store is caused by the trace() call in
disp_destructor(). This call to trace() should probably be a NOOP in a
non-debug build
but we still need to fix reported problem so DEBUG builds don't hit the
problem. I will raise a separate defect to disable the trace calls in
non-DEBUG builds.
I have resolved the problem reported under defect 35872 by defining a
new boolean flag rshutdown_started in COMG which is set to false by
COM RINIT
routine and true by the COM RSHUTDOWN routine. This allows the COM code
to avoid reference to the object store if invoked during request shutdown.
The trace calls themselves have been modified to add the object address
to allow entries referring to the same object to be correlated even if
the object name is not
available.
Fixing code in disp_destructor revealed a similar issue in FETCH_DISP
macro which is fixed in a same way.
The full patch; built against CVS HEAD is attached.
### Eclipse Workspace Patch 1.0
#P php6-cvs
Index: ext/com_dotnet/com_extension.c
===
RCS file: /repository/php-src/ext/com_dotnet/com_extension.c,v
retrieving revision 1.24
diff -u -r1.24 com_extension.c
--- ext/com_dotnet/com_extension.c 1 Jan 2007 09:29:21 - 1.24
+++ ext/com_dotnet/com_extension.c 2 Mar 2007 13:58:14 -
@@ -315,6 +315,7 @@
*/
PHP_RINIT_FUNCTION(com_dotnet)
{
+ COMG(rshutdown_started) = 0;
return SUCCESS;
}
/* }}} */
@@ -328,6 +329,7 @@
php_com_dotnet_rshutdown(TSRMLS_C);
}
#endif
+ COMG(rshutdown_started) = 1;
return SUCCESS;
}
/* }}} */
Index: ext/com_dotnet/com_wrapper.c
===
RCS file: /repository/php-src/ext/com_dotnet/com_wrapper.c,v
retrieving revision 1.14
diff -u -r1.14 com_wrapper.c
--- ext/com_dotnet/com_wrapper.c24 Feb 2007 16:25:53 - 1.14
+++ ext/com_dotnet/com_wrapper.c2 Mar 2007 13:58:14 -
@@ -92,12 +92,17 @@
# define TSRMLS_FIXED()
#endif
-#define FETCH_DISP(methname) \
- TSRMLS_FIXED() \
- php_dispatchex *disp = (php_dispatchex*)This; \
- trace( PHP:%s %s\n, Z_OBJCE_P(disp-object)-name, methname); \
- if (GetCurrentThreadId() != disp-engine_thread) \
- return RPC_E_WRONG_THREAD;
+#define FETCH_DISP(methname)
\
+ TSRMLS_FIXED()
\
+ php_dispatchex *disp = (php_dispatchex
[PHP-DEV] CVS Account Request: wharmby
Maintaining the COM extension. Andi Gutmans suggested I apply for access. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] COM extension defects: Patch for defect 37927
Hi Internals, Attached are details of a suggested patch for COM extension defect 37927 (http://bugs.php.net/bug.php?id=37927). All comments welcome; good or bad. Regards Andy Andy Wharmby IBM United Kingdom Limited Winchester, England SO21 2JN E-mail: [EMAIL PROTECTED] _*COM Defect 37927*_ This defect actually reports issues with 2 different event sink interfaces, DwebBrowserEvents2 and DWebbrowserEvents. So taking each one in turn: *DWebBrowserEvents2::NewWindow2 Event * This event is documented at : http://msdn.microsoft.com/workshop/browser/webbrowser/reference/ifaces/dwebbrowserevents2/newwindow2.asp The defect details an issue with the NewWindow2 event handler but my testing has shown the exact same issue with NewWindows3 event too. The problem here is that the user defined event handler is not getting called when the associated event occurs in IE and my investigations have shown this is due to a bug in the COM extension code. Examining the COM trace output with DebugView shows: [4716] T=1890 [4716] PHP:IEEventSinker InvokeEx [4716] T=1890 [4716] -- Invoke: 251 newwindow2 [10] flags=0001 args=2 [4716] T=1890 [4716] alloc zval for arg 0 VT=4009 only one of two arguemets gets processed !! [4716] T=1890 [4716] PHP:IEEventSinker InvokeEx next event So the event is reported but is only partially processed by the COM extension code; no attempt is made to call the event handler. BTW. This COM trace in com_wrapper.c, which output using OutputDebugString, is active in non-DEBBUG builds unlike all other uses in the PHP code base. I am not sure what the cost of these calls is but I would have expected such calls to be enabled in DEBUG builds only. Easily fixed and I will put together a fix unless anyone sees a need for this trace in non-DEBUG, i.e production builds. Anyway the reason for the partial request processing is because the code is trapping in php_com_wrap_variant() when processing the input arguments for the NewWindow2 (and NewWindow3) event. The NewWIndow2 event is defined on MSDN as follows: void NewWindow2(IDispatch **ppDisp, VARIANT_BOOL *Cancel); i.e a double indirect pointer to a IDispatch. However, this argument is defined for output. The VT_DISPATCH variant addressed has its type set but its value is NULL , i.e V_DISPATCH(var addr) == NULL . So when php_com_wrap_variant() attempts a call on the IDispatch interface to get type information and the PHP thread traps. Easily fixed by checking for a NULL IDispatch interface pointer prior to call. Required patch to fix is here: http://www.pastebin.ca/329136 This fixes the problem of the event handler not being called in the first place. However any attempt by a NewWindow2/3 event handler to cancel the navigation, i.e by setting $cancel == TRUE, will still fail until the fix for defect 345764 is dropped (http://bugs.php.net/bug.php?id=34564). Frank is currently reviewing this fix. *DWebBrowserEvents::NewWindow Event * This event is documented at : http://msdn.microsoft.com/workshop/browser/webbrowser/reference/ifaces/dwebbrowserevents/newwindow.asp. However, the MSDN description does include the following warning: This interface is obsolete; use the DWebBrowserEvents2 interface instead. The DWebBrowserEvents2 interface provides more control over the WebBrowser control than the DWebBrowserEvents interface. From my investigations I believe this interface has fallen into a state of disrepair and should not be used anymore. I can see no reason why anyone would need to use DWebBrowserEvents in preference to DWebBrowserEvents2; given that the minimum requirements, IE and Windows levels, are the same in both cases. During my investigation I found 2 problems which appear IE related and not issues in the COM extension itself: (1) The interface defines NewWindow as an event that takes just 2 arguments. However it appears it actually now takes 6 !!. The COM trace shows: [7060] T=1de4 [7060] PHP:IEEventSinker InvokeEx [7060] T=1de4 [7060] -- Invoke: 107newwindow [9] flags=0001 args=6 [7060] T=1de4 [7060] alloc zval for arg 0 VT=0008 [7060] T=1de4 [7060] alloc zval for arg 1 VT=0003 [7060] T=1de4 [7060] alloc zval for arg 2 VT=0008 [7060] T=1de4 [7060] alloc zval for arg 3 VT=400c [7060] T=1de4 [7060] alloc zval for arg 4 VT=0008 [7060] T=1de4 [7060] alloc zval for arg 5 VT=400b [7060] T=1de4 [7060] arguments processed, prepare to do some work [7060] T=1de4 [7060] function called ok So defining the event handler as described on the MSDN site will result in unpredictable behaviour; certainly navigation will not be prevented with the supplied testcase. The 6th argument is of type VT_VOOL so I am guessing that's the cancel argument. By adding dummy arguments to the event handler as follows: function NewWindow($dum1, $dum2, $dum3, $dum4
[PHP-DEV] COM extension defects: Possible patch for defect 35464
Hi Internals Attached is a suggested patch for COM defect 35464 (http://bugs.php.net/bug.php?id=34564) Any comments; good or bad welcome. Regards Andy Andy Wharmby IBM United Kingdom Limited Winchester, England SO21 2JN E-mail: [EMAIL PROTECTED] COM defect 35464 The following details 2 reasons why in/out parameters don't work; one caused by a problem with tetscase another a defect in COM extension code. The testcase supplied by the raiser implements the DWebBrowserEvents2 interface to register an handler for the BeforeNaviagte2 event. This handler is passed just one in/out parameter; a VARIANT_BOOL which can be set to TRUE by the handler to cancel the navigation operation. Doing so in PHP currently has no affect, for the 2 reasons I will describe below, so the navigation completes and the specified page is displayed by IE. Issue 1 == The users code is attempting to modify the variant directly in PHP code as follows: $cancel = true; rather than using the COM method variant_set as follows: variant_set($cancel, true); With this correction to the PHP script in place the variant for $cancel is correctly changed to TRUE but IE still navigates to the requested page so the modification is having no effect. The reason for this is the subject of issue 2 below. Issue 2 == When an event notification is received by the COM extension disp_invokeex() processes the incoming parameters (variants) by taking each one and wrapping it in a php_com_dotnet_object object. At this time a COPY of the incoming variant is embedded into the php_com_dotnet_object so we immediately have 2 copies of the variant and it is this copy in the php_com_dotnet_object which is processed (get and set) by the PHP code. I see no code that checks for modification to our copy in the php_com_dotnet_object before returning to the caller (in this case IE) so modification to in/out parameters by the PHP code has no affect. Given that the code copies an incoming variant in php_com_wrap_variant() I would have expected to see some code prior to return in disp_invokeex() which checks for modifications to in/out parameters and copies any modified values back to the callers copy of the variant. I have hacked some code as follows: com_wrapper.c: http://www.pastebin.ca/328026 com_variant.c: http://www.pastebin.ca/328022 com_misc.c: http://www.pastebin.ca/328025 php_com_dotnet_internal.h: http://www.pastebin.ca/328027 The new code works as follows: (1) When a variant is modified by a call to variant_set() in com_variant.c a new flag (obj-modified) is set in the php_com_dotnet_object. (2) After a successful call to a event handler new code in com_wrapper.c function disp_invokeex() checks each of the event handlers arguments (php_com_dotnet_object's ) to see if any of their embedded variants have been modified. If so and the argument passed into the event handler was passed by reference then the value in the embedded variant is copied to the callers copy by a call to a new function php_com_copy_variant() defined in com_variant.c. With this patch applied when the supplied testcase is run the navigation is now cancelled as expected. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] COM extension defects: defect 39930
Hi Internals As promised back in December I have now found some time to look at the COM defects to help reduce the backlog. I have now investigated the first of these; http://bugs.php.net/bug.php?id=39930 and have updated the defect with my findings. In this case I believe the defect is in Word rather than PHP or the COM extension. Can someone with enough karma please take a look at my comments and if they are OK return the defect as bogus. Many thanks. Regards Andy Andy Wharmby IBM United Kingdom Limited Winchester, England SO21 2JN E-mail: [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Re: Query on assert_options() api
Andy Wharmby wrote:
Hi Internals
A colleague of mine is currently working on creating new test
cases to improve the coverage of the PHP test cases and whilst
attempting to
write new tests for the assert extension hit on a problem when
attempting to query the current setting of ASSERT_CALLBACK. using the
assert_options() api.
With the following simple testcase :
?php
function andy()
{
echo andy called\n;
}
$o = assert_options(ASSERT_CALLBACK, andy); /* SET */
$n = assert_options(ASSERT_CALLBACK); /* INQ */
echo old setting is $o\n;
echo new setting is $n\n; ?
The expected result was that the old (if any) and new setting of the
callback function name would be displayed but the actual results was
old setting is 1
new setting is 1
Looking at the code this is no surprise as the code in assert.c which
processes these requests unconditionally returns with RETURN_TRUE.
For all other assert options other than assert_options() function
returns the old value of the specified option as documented in the PHP
manual.
Further investigation uncovered that the code actually did behave in
the way we expected until it was changed to accept the array($obj,
methodname)
syntax back in July 2001 (revision 1.32 of assert.c). At this time the
return was changed to be unconditionally TRUE.
Unfortunately this makes writing a test case to query the current
setting of the ASSERT_CALLBACK option impossible as assert_options()
no longer returns
any useable information for this option, i.e to code a testcase as
above that sets a value and then checks its set as expected.
If the code is modified as in the attached patch to instead return the
ZVAL for the ASSERT_CALLBACK option then we are able create the new
testcase to
verify the assert_options() api. However, I am concerned there is a
good reason this was not done when the code was changed back in
2001.Can anyone think
of a good reason why returning the zval on this api is not a good idea ?
Any comments on my proposed fix appreciated.
Regards
Andy
Andy Wharmby
IBM United Kingdom Limited
Hi All,
I have now completed further testing on the patch I posted
yesterday and have uncovered a further defect that will need addressing
if the
earlier fix is accepted.
With the code modified such that assert_options(ASSERT_CALLBACK) returns
the current setting of the option it uncovers a problem with
a recent change to assert.c to fix defect 39718 (
http://bugs.php.net/bug.php?id=39718).
With the following simple script
?php
function andy()
{
echo andy called;
}
function default_callback()
{
echo default_calback called;
}
/* Check php.ini setting set */
$o = assert_options(ASSERT_CALLBACK);
echo Initial setting is \$o\ ...it should be default_callback!!!\n;
assert(0);
/* modify callback */
$o= assert_options(ASSERT_CALLBACK, andy);
assert(0);
?
and the following php.ini setting:
assert.callback=default_callback
then the expected result is:
Initial setting is default_callback ...it should be default_callback!!!
default_calback called
Warning: assert(): Assertion failed in
C:\Eclipse-PHP\workspace\Testcases\assertbug.php on line 16
andy called
Warning: assert(): Assertion failed in
C:\Eclipse-PHP\workspace\Testcases\assertbug.php on line 20
the actual result though is
Initial setting is...it should be default_callback!!!
default_calback called
Warning: assert(): Assertion failed in
C:\Eclipse-PHP\workspace\Testcases\assert
bug.php on line 16
andy called
Warning: assert(): Assertion failed in
C:\Eclipse-PHP\workspace\Testcases\assert
bug.php on line 20
Rather than getting the php.ini setting of default_callback returned
NULL is returned instead. Issuing assert() calls the expected callback
OK though.
The problem is that the value of the global ASSERTG(cb) is not copied
to ASSERTG(callback) until the first call to assert() by a request
so if the script includes a query on the setting BEFORE the first call
to assert() then the value returned is the default INI setting, i.e.
NULL, rather
than any value specified in php.ini. The issues is easily addressed by
moving the code that populates ASSERTG(callback) to a new RINIT function
in assert.c.
The modified patches for all the necessary changes to assert.c and
basic_functions.c are attached. If the fix is accepted I will get my
colleague to drop the
updated PHPT tests into CVS.
If a defect needs to be opened for this issue just let me know and I
will do so.
Regards
Andy
Andy Wharmby
IBM United Kingdom Limited
--- assert.c.old2006-12-03 17:30:54.0 +
+++ assert.c2006-12-21 13:26:35.71875 +
@@ -114,6 +114,17 @@
return SUCCESS;
}
+PHP_RINIT_FUNCTION(assert)
+{
+ if (ASSERTG(cb)) {
+ MAKE_STD_ZVAL(ASSERTG(callback));
+ ZVAL_STRING(ASSERTG(callback), ASSERTG(cb), 1);
+ }
+
+ return SUCCESS
Re: [PHP-DEV] Moving COM, Socket mhash to PECL
Wez Furlong wrote: On 12/8/06, Andi Gutmans [EMAIL PROTECTED] wrote: As far as finding a maintainer is concerned, we can check again with Wez. If he doesn't have time we should probably be able to find someone who can look into some of those bugs. Despite bugs, I've never had issues with it. I'm currently (way!) too busy to look at it, but it sounds like Frank and Pierre have volunteered some time. I'll happily review patches. --Wez. Hi Internals I am still pretty new to PHP but having spent the last couple of months reviewing the PHP code and getting to know how PHP ticks I would be happy to volunteer some time take a look at some of the outstanding issues with the COM extension. I am about to go on vacation for a couple of weeks but I will certainly have time early in 2007 to investigate some of the OPEN defects. As a couple of others (Frank and Pierre) have also volunteered time whats the best way to work this so that we make best use of our time. Should I just add add a comment to a defect when I start investigating it and then post any fixes I come up with to list for review ? Regards Andy Andy Wharmby IBM United Kingdom Limited -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Query on assert_options() api
Hi Internals
A colleague of mine is currently working on creating new test
cases to improve the coverage of the PHP test cases and whilst
attempting to
write new tests for the assert extension hit on a problem when
attempting to query the current setting of ASSERT_CALLBACK. using the
assert_options() api.
With the following simple testcase :
?php
function andy()
{
echo andy called\n;
}
$o = assert_options(ASSERT_CALLBACK, andy); /* SET */
$n = assert_options(ASSERT_CALLBACK); /* INQ */
echo old setting is $o\n;
echo new setting is $n\n;
?
The expected result was that the old (if any) and new setting of the
callback function name would be displayed but the actual results was
old setting is 1
new setting is 1
Looking at the code this is no surprise as the code in assert.c which
processes these requests unconditionally returns with RETURN_TRUE.
For all other assert options other than assert_options() function
returns the old value of the specified option as documented in the PHP
manual.
Further investigation uncovered that the code actually did behave in the
way we expected until it was changed to accept the array($obj,
methodname)
syntax back in July 2001 (revision 1.32 of assert.c). At this time the
return was changed to be unconditionally TRUE.
Unfortunately this makes writing a test case to query the current
setting of the ASSERT_CALLBACK option impossible as assert_options() no
longer returns
any useable information for this option, i.e to code a testcase as
above that sets a value and then checks its set as expected.
If the code is modified as in the attached patch to instead return the
ZVAL for the ASSERT_CALLBACK option then we are able create the new
testcase to
verify the assert_options() api. However, I am concerned there is a
good reason this was not done when the code was changed back in 2001.Can
anyone think
of a good reason why returning the zval on this api is not a good idea ?
Any comments on my proposed fix appreciated.
Regards
Andy
Andy Wharmby
IBM United Kingdom Limited
--- assert.c2006-12-03 17:30:54.0 +
+++ assert.c.new2006-12-20 14:55:03.259056200 +
@@ -286,6 +286,11 @@
break;
case ASSERT_CALLBACK:
+ if (ASSERTG(callback) != NULL) {
+ RETVAL_ZVAL(ASSERTG(callback), 1, 0);
+ } else {
+ RETVAL_NULL();
+ }
if (ac == 2) {
if (ASSERTG(callback)) {
zval_ptr_dtor(ASSERTG(callback));
@@ -293,7 +298,7 @@
ASSERTG(callback) = *value;
zval_add_ref(value);
}
- RETURN_TRUE;
+ return;
break;
default:
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Question on thread safety
Stanislav Malyshev wrote: So finally to my question. Is it the intention of TSRMc. to allow ts_allocate_id() to be called at any time or is there an unwritten rule that it should only ever called during php startup ? If its the former then I I think it gets called only on startup. I also think it was the intent, though there is no safeguard as far as I can see against calling it in run-time, but no module does it and it doesn't make sense to do it in other place than startup. I myself see no reason why extension writers should be restricted from calling ts_allocate_id() outside PHP startup so believe the code needs Well, the reason is that if you want to use TSRM globals, you have to allocate ID before you do basically anything with them. Startup is a good place for that. If you don't need globals, then you should not call it at all. The situation where in the mid-run you suddenly remember you need globals seems quite unrealistic to me. Of course, if you can describe scenario when you would really need it in mid-run or it would make sense to allocate ID in mid-run, then I guess this should be fixed or at least safeguarded. I did not have any particular scenario in mind here as I too could not come up with a sensible reason to allow calls to ts_allocate_id() outside initialization. Initially I thought of the case of a user script loading an extension using dl() but soon found out this is policed and not allowed if ZTS is enabled. The reason I assumed that ts_allocate_id() was designed to be called at any time was the fact that the code is wrapped in a mutex, which lead to my concerns about ts_resource_ex(). If ts_allocate_id() is not designed to be called outside startup then my concerns about ts_resource_ex() are unfounded. However, the mutex acquire and release calls in ts_allocate_id() are therefore unnecessary and should be removed. However, I do believe this restriction should be policed to fail any calls outside startup. I see nothing in the code to stop a extension writer calling ts_allocate_id at runtime. Why would anyone do this ? So TSRM global storage is only allocated when an extension is actually needed rather than when a new thread is started. What if a user has an extension that only gets called in some exceptional circumstance and they design it so the ts_allocate_id() call is made on the first call to the extension to save storage being allocated for threads until its needed. Unlikely I know but sometimes users do what you least expect so the code should protect them wherever possible from them doing something which will: (a) cause them grief, and (b) probably lead them into raising a bogus defect and waste someones time diagnosing the problem A simple check in the code could prevent all this. There are further routines in TSRM.c which also acquire the tsmm_mutex were the reason for this is not clear given current usage: * ts_free_id(). Only called at MSHUTDOWN when single threaded so no apparent need for mutex. Again easily policed to ensure calls outside startup/shutdown not allowed. * ts_free_worker_threads(): Only called by php_module_shutdown when single threaded so no need for mutex. * ts_resource_ex: Needs mutex whilst it updates tsrm_tls_table for a new thread but it looks like it keeps mutex longer than it need do. By reworking this routine and allocate_new_resource() I believe the time the mutex is held could be reduced. I am happy to work on a patch for all this and will raise a defect with patch when I have something worthy of consideration.. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Question on thread safety
that would stop an extension writer from calling ts_allocate_id() outside of MINIT, e.g in request initialization (RINIT). If this did happen then problems are inevitable in ZTS enabled builds and the code should be fixed before it causes issues which could be difficult to diagnose. So finally to my question. Is it the intention of TSRMc. to allow ts_allocate_id() to be called at any time or is there an unwritten rule that it should only ever called during php startup ? If its the former then I believe the code is broken as it is and should be fixed before it causes any problems. I am happy to investigate possible solutions. If its the latter then I believe the rule needs to be policed by code and any attempt to call ts_allocate_id() after startup failed gracefully. I myself see no reason why extension writers should be restricted from calling ts_allocate_id() outside PHP startup so believe the code needs to be fixed but I would appreciate the views of others with more experience into the workings of the code on how this potential problem should be addressed before I spend time working on any possible fix. Regards, Andy Andy Wharmby IBM United Kingdom Limited -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
