Re: [PHP-DEV] [PATCH] readline extension bug fixes and enhancements
Hello On Mar 20, 2012, at 9:43 PM, Johannes Schlüter wrote: Have you read Nikita's and my comments on your mail from last week? I just did. There was a problem with my mailing list subscription (wasn't confirmed) so I thought it didn't go through. But the good thing is that I had ported it to 5.4 and found the memory corruption bug. Here are my answers to your comments: On Mar 14, 2012, at 10:28 PM, Johannes Schlüter wrote: A) for licensing reasons we should try to keep as few readline only things as possible in there (gpl vs. php license) I understand the issue of licensing but the extension already has readline-specific functionality so I thought why not. However if this is going to be a problem then I can take out all readline-sepcific functions into a separate module altogether and keep this one for editline functions only. B) thread safty isn't an issue for readline but you still should do the init and deinit in rinit/rshutdown not minit/mshutdown, probably also do only bind_key_functions =NULL in rinit and init the hashtable on demand later. Roger that. C) don't compare the r result of zend_hash_find and others against -1 or such but use SUCCESS/FAILURE Roger that. D) i don't really like new features in 5.3 anymore at this stage This last patch is ported to 5.4. E) please log a bug and attach the patch so we can track this Roger that. After I clear out which direction to take based on your answers. And as for Nikita's comments: On Mar 14, 2012, at 10:47 PM, Nikita Popov wrote: May I ask where you got this pattern for copying zvals from? Is this kind of code shown in some tutorial / manual / etc? Such code is used all other the place, instead of the MAKE_COPY_ZVAL macro and I wonder why. Doing manual copying has a good chance of leaking memory. Just from glancing I'd say that your code will leak if arg has a refcount 1 (but I could be wrong) as it does not PINIT the zval after it was copied. Two points here: A) The new patch does not use this at all because it really isn't needed B) I got this from the same readline.c file lines 521 and 574 and 579. But I will change it accordingly -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] [PATCH] readline extension bug fixes and enhancements
All,
I was recently involved in a project that relied heavily on readline to provide
console text input capabilities. However I soon noticed that the current
readline extension has a serious bug and is lacking some important
functionality.
This patch applies only to PHP compiled with libreadline (and don't have an
effect if compiled with libeditline). It adds/modifies/fixes the following:
- Fixes memory corruption caused by directly setting rl_line_buffer to new
buffer and not using rl_replace_line().
- Fixes a bad combination of preprocessor #if that would cause
readline_on_new_line() implementation not to compile when using libreadline
only.
- Adds support for rl_bind_key function by exposing:
function readline_bind_key_function($key, $callback_func)
where:
$key: Key code to bind to.
$callback_func: A callback function in the form:
function callback($key, $count) where $key and $count are the same parameters
passed from rl_bind_key()
Setting $callback_func to null will cause the binding to be removed for $key.
- Modifies the behavior of readline_info() to allow for setting the readline
properties point and end (it used to only read them but not set them).
Patch below:
--- php-5.4.0/ext/readline/readline.c 2012-01-01 15:15:04.0 +0200
+++ php-5.4.0-patched/ext/readline/readline.c 2012-03-19 09:09:27.388174973
+0200
@@ -58,9 +58,13 @@ PHP_FUNCTION(readline_callback_read_char
PHP_FUNCTION(readline_callback_handler_remove);
PHP_FUNCTION(readline_redisplay);
PHP_FUNCTION(readline_on_new_line);
+PHP_FUNCTION(readline_bind_key_function);
static zval *_prepped_callback = NULL;
+#endif
+#if HAVE_LIBREADLINE
+static HashTable _bind_key_functions_ht;
#endif
static zval *_readline_completion = NULL;
@@ -121,10 +125,19 @@ ZEND_END_ARG_INFO()
ZEND_BEGIN_ARG_INFO(arginfo_readline_redisplay, 0)
ZEND_END_ARG_INFO()
+#endif
+#if HAVE_RL_ON_NEW_LINE | HAVE_LIBREADLINE
ZEND_BEGIN_ARG_INFO(arginfo_readline_on_new_line, 0)
ZEND_END_ARG_INFO()
#endif
+
+#if HAVE_LIBREADLINE
+ZEND_BEGIN_ARG_INFO_EX(arginfo_readline_bind_key_function, 0, 0, 2)
+ ZEND_ARG_INFO(0, key)
+ ZEND_ARG_INFO(0, funcname)
+ZEND_END_ARG_INFO()
+#endif
/* }}} */
/* {{{ module stuff */
@@ -145,9 +158,12 @@ static const zend_function_entry php_rea
PHP_FE(readline_callback_handler_remove,
arginfo_readline_callback_handler_remove)
PHP_FE(readline_redisplay, arginfo_readline_redisplay)
#endif
-#if HAVE_RL_ON_NEW_LINE
+#if HAVE_RL_ON_NEW_LINE | HAVE_LIBREADLINE
PHP_FE(readline_on_new_line, arginfo_readline_on_new_line)
#endif
+#if HAVE_LIBREADLINE
+ PHP_FE(readline_bind_key_function, arginfo_readline_bind_key_function)
+#endif
PHP_FE_END
};
@@ -170,6 +186,9 @@ ZEND_GET_MODULE(readline)
PHP_MINIT_FUNCTION(readline)
{
+#if HAVE_LIBREADLINE
+ zend_hash_init(_bind_key_functions_ht, 255, NULL, ZVAL_PTR_DTOR, 1);
+#endif
using_history();
return PHP_MINIT(cli_readline)(INIT_FUNC_ARGS_PASSTHRU);
}
@@ -193,6 +212,9 @@ PHP_RSHUTDOWN_FUNCTION(readline)
}
#endif
+#if HAVE_LIBREADLINE
+ zend_hash_destroy(_bind_key_functions_ht);
+#endif
return SUCCESS;
}
@@ -265,12 +287,20 @@ PHP_FUNCTION(readline_info)
if (value) {
/* XXX if (rl_line_buffer)
free(rl_line_buffer); */
convert_to_string_ex(value);
- rl_line_buffer = strdup(Z_STRVAL_PP(value));
+ rl_replace_line(Z_STRVAL_PP(value), 1);
}
RETVAL_STRING(SAFE_STRING(oldstr),1);
} else if (!strcasecmp(what, point)) {
+ if (value) {
+ convert_to_long_ex(value);
+ rl_point = Z_LVAL_PP(value);
+ }
RETVAL_LONG(rl_point);
} else if (!strcasecmp(what, end)) {
+ if (value) {
+ convert_to_long_ex(value);
+ rl_end = Z_LVAL_PP(value);
+ }
RETVAL_LONG(rl_end);
#ifdef HAVE_LIBREADLINE
} else if (!strcasecmp(what, mark)) {
@@ -621,7 +651,7 @@ PHP_FUNCTION(readline_redisplay)
#endif
-#if HAVE_RL_ON_NEW_LINE
+#if HAVE_RL_ON_NEW_LINE | HAVE_LIBREADLINE
/* {{{ proto void readline_on_new_line(void)
Inform readline that the cursor has moved to a new line */
PHP_FUNCTION(readline_on_new_line)
@@ -632,6 +662,72 @@ PHP_FUNCTION(readline_on_new_line)
#endif
+/* {{{ proto bool readline_bind_key_function(string funcname)
+ Readline rl_bind_key */
+
+static int _readline_bind_key_cb(int count, int key)
+{
+ zval *params[2];
+ TSRMLS_FETCH();
+
+ params[0]=_readline_long_zval(key);
+ params[1]=_readline_long_zval(count);
+
+ zval
[PHP-DEV] [PATCH] readline extension rl_bind_key addition and other enhancements
I was recently involved in a project that relied heavily on readline to provide
console text input capabilities. However I soon noticed that the current
readline is lacking some important functionality.
This patch applies only to PHP compiled with libreadline (and don't have an
effect if compiled with libeditline). It adds/modified the following:
- Adds support for rl_bind_key function by exposing:
function readline_bind_key_function($key, $callback_func)
where:
$key: Key code to bind to.
$callback_func: A callback function in the form:
function callback($key, $count) where $key and $count are the same parameters
passed from rl_bind_key()
Setting $callback_func to null will cause the binding to be removed for $key.
- Modifies the behavior of readline_info() to allow for setting the readline
properties point and end (it used to only read them but not set them).
Patch below:
--- php-5.3.10/ext/readline/readline.c 2012-01-01 15:15:04.0 +0200
+++ php-5.3.10-patched/ext/readline/readline.c 2012-03-13 09:35:43.0
+0200
@@ -57,9 +57,12 @@ PHP_FUNCTION(readline_callback_read_char
PHP_FUNCTION(readline_callback_handler_remove);
PHP_FUNCTION(readline_redisplay);
PHP_FUNCTION(readline_on_new_line);
+PHP_FUNCTION(readline_bind_key_function);
static zval *_prepped_callback = NULL;
+static HashTable *_bind_key_functions_ht = NULL;
+
#endif
static zval *_readline_completion = NULL;
@@ -121,6 +124,12 @@ ZEND_END_ARG_INFO()
ZEND_BEGIN_ARG_INFO(arginfo_readline_on_new_line, 0)
ZEND_END_ARG_INFO()
+
+ZEND_BEGIN_ARG_INFO_EX(arginfo_readline_bind_key_function, 0, 0, 2)
+ ZEND_ARG_INFO(0, key)
+ ZEND_ARG_INFO(0, funcname)
+ZEND_END_ARG_INFO()
+
#endif
/* }}} */
@@ -142,6 +151,7 @@ static const zend_function_entry php_rea
PHP_FE(readline_callback_handler_remove,
arginfo_readline_callback_handler_remove)
PHP_FE(readline_redisplay, arginfo_readline_redisplay)
PHP_FE(readline_on_new_line, arginfo_readline_on_new_line)
+ PHP_FE(readline_bind_key_function, arginfo_readline_bind_key_function)
#endif
PHP_FE_END
};
@@ -165,6 +175,9 @@ ZEND_GET_MODULE(readline)
PHP_MINIT_FUNCTION(readline)
{
+ ALLOC_HASHTABLE(_bind_key_functions_ht);
+ zend_hash_init(_bind_key_functions_ht, 255, NULL, ZVAL_PTR_DTOR, 0);
+
using_history();
return SUCCESS;
}
@@ -181,6 +194,9 @@ PHP_RSHUTDOWN_FUNCTION(readline)
zval_ptr_dtor(_prepped_callback);
_prepped_callback = 0;
}
+
+ zend_hash_destroy(_bind_key_functions_ht);
+ FREE_HASHTABLE(_bind_key_functions_ht);
#endif
return SUCCESS;
@@ -254,8 +270,16 @@ PHP_FUNCTION(readline_info)
}
RETVAL_STRING(SAFE_STRING(oldstr),1);
} else if (!strcasecmp(what, point)) {
+ if (value) {
+ convert_to_long_ex(value);
+ rl_point = Z_LVAL_PP(value);
+ }
RETVAL_LONG(rl_point);
} else if (!strcasecmp(what, end)) {
+ if (value) {
+ convert_to_long_ex(value);
+ rl_end = Z_LVAL_PP(value);
+ }
RETVAL_LONG(rl_end);
#ifdef HAVE_LIBREADLINE
} else if (!strcasecmp(what, mark)) {
@@ -612,6 +636,74 @@ PHP_FUNCTION(readline_on_new_line)
}
/* }}} */
+/* {{{ proto bool readline_bind_key_function(string funcname)
+ Readline rl_bind_key */
+
+static int _readline_bind_key_cb(int count, int key)
+{
+ zval *params[2];
+ TSRMLS_FETCH();
+
+ params[0]=_readline_long_zval(key);
+ params[1]=_readline_long_zval(count);
+
+ zval **_callback_func = NULL;
+ long _key = key;
+ int r = zend_hash_find(_bind_key_functions_ht, (char *) _key,
sizeof(_key), (void **)_callback_func);
+ if (r == -1)
+ return -1;
+
+ int _return_int = 0;
+ zval *_callback_return = NULL;
+ MAKE_STD_ZVAL(_callback_return);
+ if (call_user_function(CG(function_table), NULL, *_callback_func,
_callback_return, 2, params TSRMLS_CC) == SUCCESS) {
+ _return_int = _callback_return-value.lval;
+ zval_dtor(_callback_return);
+ FREE_ZVAL(_callback_return);
+ }
+
+ return _return_int;
+}
+
+PHP_FUNCTION(readline_bind_key_function)
+{
+ long key;
+ zval *arg = NULL;
+ char *name = NULL;
+
+ if (FAILURE == zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, l!z,
key, arg)) {
+ RETURN_FALSE;
+ }
+
+ if (Z_TYPE_P(arg) == IS_NULL) {
+ zend_hash_del(_bind_key_functions_ht, (char *)key,
sizeof(key));
+ rl_bind_key(key, rl_insert);
+ }
+ else {
+ if
