[libvirt] [libvirt-php PATCH 0/3] Fix PHP5 compatibilty issues.
With the patches that landed support for PHP7 some API calls were not behaving correctly when compiled against PHP5. The most obvious case is where connection to e.g. esx host, libvirt_connect wasn't properly reading out credentials from Zeng HashTable which resulted in failure to authenticate. This issue was caused mainly be the differnce in Zend's implementaion of zend_hash_get_current_data_ex which in PHP7 takes a pointer to zval whereas in PHP5 it takes pointer to pointer to zval. To address this, the first commit adds macros that abstract away such implementation details and the remaining commits make use of that. Dawid Zamirski (3): Define macros for looping php hash tables. libvirt_connect: use loop macros to read cred info. use VIRT_FOREACH macros everywhere. src/libvirt-php.c | 205 +++++++++++++++++------------------------------------- src/libvirt-php.h | 6 ++ 2 files changed, 69 insertions(+), 142 deletions(-) -- 2.7.4
Since PHP5 and 7 differ slightly on how looping over php hash tables is done, those macros were added to abstract away those differences and avoid using ifdefs for each php version. --- src/libvirt-php.c | 31 +++++++++++++++++++++++++++++++ src/libvirt-php.h | 6 ++++++ 2 files changed, 37 insertions(+) diff --git a/src/libvirt-php.c b/src/libvirt-php.c index 85bfcc2..a105dd3 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -83,6 +83,21 @@ typedef size_t strsize_t; #define VIRT_ADD_ASSOC_STRING_EX(_arg, _key, _key_len, _value) \ add_assoc_string_ex(_arg, _key, _key_len, _value) +#define VIRT_FOREACH(_ht, _pos, _zv) \ + for (zend_hash_internal_pointer_reset_ex(_ht, &_pos); \ + (_zv = zend_hash_get_current_data_ex(_ht, &_pos)) != NULL; \ + zend_hash_move_forward_ex(_ht, &_pos)) \ + +#define VIRT_FOREACH_END(_dummy) + +#define VIRT_HASH_CURRENT_KEY_INFO(_ht, _pos, _idx, _info) \ + do { \ + zend_string *tmp_key_info; \ + _info.type = zend_hash_get_current_key_ex(_ht, &tmp_key_info, &_idx, &_pos); \ + _info.name = ZSTR_VAL(tmp_key_info); \ + _info.length = ZSTR_LEN(tmp_key_info); \ + } while(0) + #else /* PHP_MAJOR_VERSION < 7 */ typedef int strsize_t; typedef long zend_long; @@ -110,6 +125,22 @@ typedef unsigned long zend_ulong; #define VIRT_ADD_ASSOC_STRING_EX(_arg, _key, _key_len, _value) \ add_assoc_string_ex(_arg, _key, _key_len, _value, 1) +#define VIRT_FOREACH(_ht, _pos, _zv) \ + { \ + zval **pzv = &_zv; \ + for (zend_hash_internal_pointer_reset_ex(_ht, &_pos); \ + zend_hash_get_current_data_ex(_ht, (void **) &pzv, &_pos) == SUCCESS; \ + zend_hash_move_forward_ex(_ht, &_pos)) { \ + _zv = *pzv; + +#define VIRT_FOREACH_END(_dummy) \ + }} + +#define VIRT_HASH_CURRENT_KEY_INFO(_ht, _pos, _idx, _info) \ + do { \ + _info.type = zend_hash_get_current_key_ex(_ht, &_info.name, &_info.length, &_idx, 0, &_pos); \ + } while(0) + #endif /* PHP_MAJOR_VERSION < 7 */ /* ZEND thread safe per request globals definition */ diff --git a/src/libvirt-php.h b/src/libvirt-php.h index f1ba9c9..6e61fea 100644 --- a/src/libvirt-php.h +++ b/src/libvirt-php.h @@ -315,6 +315,12 @@ typedef struct _php_libvirt_cred_value { unsigned int resultlen; } php_libvirt_cred_value; +typedef struct _php_libvirt_hash_key_info { + char *name; + uint length; + uint type; +} php_libvirt_hash_key_info; + /* Private definitions */ int vnc_refresh_screen(char *server, char *port, int scancode); int vnc_send_keys(char *server, char *port, char *keys); -- 2.7.4
On 06.07.2016 23:42, Dawid Zamirski wrote:
Since PHP5 and 7 differ slightly on how looping over php hash tables is done, those macros were added to abstract away those differences and avoid using ifdefs for each php version. --- src/libvirt-php.c | 31 +++++++++++++++++++++++++++++++ src/libvirt-php.h | 6 ++++++ 2 files changed, 37 insertions(+)
diff --git a/src/libvirt-php.c b/src/libvirt-php.c index 85bfcc2..a105dd3 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -83,6 +83,21 @@ typedef size_t strsize_t; #define VIRT_ADD_ASSOC_STRING_EX(_arg, _key, _key_len, _value) \ add_assoc_string_ex(_arg, _key, _key_len, _value)
+#define VIRT_FOREACH(_ht, _pos, _zv) \ + for (zend_hash_internal_pointer_reset_ex(_ht, &_pos); \ + (_zv = zend_hash_get_current_data_ex(_ht, &_pos)) != NULL; \ + zend_hash_move_forward_ex(_ht, &_pos)) \ + +#define VIRT_FOREACH_END(_dummy) + +#define VIRT_HASH_CURRENT_KEY_INFO(_ht, _pos, _idx, _info) \ + do { \ + zend_string *tmp_key_info; \ + _info.type = zend_hash_get_current_key_ex(_ht, &tmp_key_info, &_idx, &_pos); \ + _info.name = ZSTR_VAL(tmp_key_info); \ + _info.length = ZSTR_LEN(tmp_key_info); \ + } while(0) + #else /* PHP_MAJOR_VERSION < 7 */ typedef int strsize_t; typedef long zend_long; @@ -110,6 +125,22 @@ typedef unsigned long zend_ulong; #define VIRT_ADD_ASSOC_STRING_EX(_arg, _key, _key_len, _value) \ add_assoc_string_ex(_arg, _key, _key_len, _value, 1)
+#define VIRT_FOREACH(_ht, _pos, _zv) \ + { \ + zval **pzv = &_zv; \ + for (zend_hash_internal_pointer_reset_ex(_ht, &_pos); \ + zend_hash_get_current_data_ex(_ht, (void **) &pzv, &_pos) == SUCCESS; \ + zend_hash_move_forward_ex(_ht, &_pos)) { \ + _zv = *pzv; + +#define VIRT_FOREACH_END(_dummy) \ + }} + +#define VIRT_HASH_CURRENT_KEY_INFO(_ht, _pos, _idx, _info) \ + do { \ + _info.type = zend_hash_get_current_key_ex(_ht, &_info.name, &_info.length, &_idx, 0, &_pos); \ + } while(0) + #endif /* PHP_MAJOR_VERSION < 7 */
/* ZEND thread safe per request globals definition */ diff --git a/src/libvirt-php.h b/src/libvirt-php.h index f1ba9c9..6e61fea 100644 --- a/src/libvirt-php.h +++ b/src/libvirt-php.h @@ -315,6 +315,12 @@ typedef struct _php_libvirt_cred_value { unsigned int resultlen; } php_libvirt_cred_value;
+typedef struct _php_libvirt_hash_key_info { + char *name; + uint length; + uint type;
Fully expanded version is preferred. Moreover, this doesn't need to be in the header file at all - esp. if the macros using it are just in .c. But since we have other typedefs there, I can live with that. One day, I'll rewrite libvirt-php so that my eyes don't bleed when I open the sources. Anyway, for now I'll fix this before pushing.
+} php_libvirt_hash_key_info; + /* Private definitions */ int vnc_refresh_screen(char *server, char *port, int scancode); int vnc_send_keys(char *server, char *port, char *keys);
Michal
Since addition of PHP7 the libvirt_connect regressed on PHP5 where it was no longer reading out credential info from the php hash table properly. This patch makes use of the loop macros that handle both PHP versions correctly and consequently fix this issue. --- src/libvirt-php.c | 33 +++++++-------------------------- 1 file changed, 7 insertions(+), 26 deletions(-) diff --git a/src/libvirt-php.c b/src/libvirt-php.c index a105dd3..e03990a 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -2298,31 +2298,13 @@ PHP_FUNCTION(libvirt_connect) creds=(php_libvirt_cred_value *)emalloc( credscount * sizeof(php_libvirt_cred_value) ); j=0; /* parse the input Array and create list of credentials. The list (array) is passed to callback function. */ -#if PHP_MAJOR_VERSION >= 7 - for (zend_hash_internal_pointer_reset_ex(arr_hash, &pointer); - (data = zend_hash_get_current_data_ex(arr_hash, &pointer)) != NULL; - zend_hash_move_forward_ex(arr_hash, &pointer)) { + VIRT_FOREACH(arr_hash, pointer, data) { if (Z_TYPE_P(data) == IS_STRING) { - zend_string *key; - if (zend_hash_get_current_key_ex(arr_hash, &key, &index, &pointer) == HASH_KEY_IS_STRING) { - PHPWRITE(ZSTR_VAL(key), ZSTR_LEN(key)); - } else { - DPRINTF("%s: credentials index %d\n", PHPFUNC, (int)index); - creds[j].type=index; - creds[j].result=(char *)emalloc( Z_STRLEN_P(data) + 1 ); - memset(creds[j].result, 0, Z_STRLEN_P(data) + 1); - creds[j].resultlen=Z_STRLEN_P(data); - strncpy(creds[j].result,Z_STRVAL_P(data),Z_STRLEN_P(data)); - j++; -#else - for (zend_hash_internal_pointer_reset_ex(arr_hash, &pointer); - zend_hash_get_current_data_ex(arr_hash, (void**) &data, &pointer) == SUCCESS; - zend_hash_move_forward_ex(arr_hash, &pointer)) { - if (Z_TYPE_P(data) == IS_STRING) { - char *key; - unsigned int key_len; - if (zend_hash_get_current_key_ex(arr_hash, &key, &key_len, &index, 0, &pointer) == HASH_KEY_IS_STRING) { - PHPWRITE(key, key_len); + php_libvirt_hash_key_info info; + VIRT_HASH_CURRENT_KEY_INFO(arr_hash, pointer, index, info); + + if (info.type == HASH_KEY_IS_STRING) { + PHPWRITE(info.name, info.length); } else { DPRINTF("%s: credentials index %d\n", PHPFUNC, (int)index); creds[j].type=index; @@ -2331,10 +2313,9 @@ PHP_FUNCTION(libvirt_connect) creds[j].resultlen = Z_STRLEN_P(data); strncpy(creds[j].result, Z_STRVAL_P(data), Z_STRLEN_P(data)); j++; -#endif } } - } + } VIRT_FOREACH_END(); DPRINTF("%s: Found %d elements for credentials\n", PHPFUNC, j); creds[0].count=j; libvirt_virConnectAuth.cbdata = (void*)creds; -- 2.7.4
This results in cleaner code and fixes other potential bugs, e.g. in get_next_free_numeric_value --- src/libvirt-php.c | 141 ++++++++++-------------------------------------------- 1 file changed, 25 insertions(+), 116 deletions(-) diff --git a/src/libvirt-php.c b/src/libvirt-php.c index e03990a..2045c59 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -3549,33 +3549,20 @@ long get_next_free_numeric_value(virDomainPtr domain, char *xpath) arr_hash = Z_ARRVAL_P(output); // array_count = zend_hash_num_elements(arr_hash); -#if PHP_MAJOR_VERSION >= 7 - for (zend_hash_internal_pointer_reset_ex(arr_hash, &pointer); - (data = zend_hash_get_current_data_ex(arr_hash, &pointer)) != NULL; - zend_hash_move_forward_ex(arr_hash, &pointer)) { + VIRT_FOREACH(arr_hash, pointer, data) { if (Z_TYPE_P(data) == IS_STRING) { - zend_string *key; - if (zend_hash_get_current_key_ex(arr_hash, &key, &index, &pointer) != HASH_KEY_IS_STRING) { - long num = -1; + php_libvirt_hash_key_info info; + VIRT_HASH_CURRENT_KEY_INFO(arr_hash, pointer, index, info); - sscanf(Z_STRVAL_P(data), "%lx", &num); -#else - for (zend_hash_internal_pointer_reset_ex(arr_hash, &pointer); - zend_hash_get_current_data_ex(arr_hash, (void**) &data, &pointer) == SUCCESS; - zend_hash_move_forward_ex(arr_hash, &pointer)) { - if (Z_TYPE_P(data) == IS_STRING) { - char *key; - unsigned int key_len; - if (zend_hash_get_current_key_ex(arr_hash, &key, &key_len, &index, 0, &pointer) != HASH_KEY_IS_STRING) { + if (info.type != HASH_KEY_IS_STRING) { long num = -1; sscanf(Z_STRVAL_P(data), "%lx", &num); -#endif if (num > max_slot) max_slot = num; } } - } + } VIRT_FOREACH_END(); efree(output); free(xml); @@ -5140,17 +5127,11 @@ PHP_FUNCTION(libvirt_domain_send_key_api) keycodes = (uint *) emalloc(count * sizeof(uint)); i = 0; - for (zend_hash_internal_pointer_reset_ex(arr_hash, &pointer); -#if PHP_MAJOR_VERSION >= 7 - (data = zend_hash_get_current_data_ex(arr_hash, &pointer)) != NULL; -#else - zend_hash_get_current_data_ex(arr_hash, (void **) &data, &pointer) == SUCCESS; -#endif - zend_hash_move_forward_ex(arr_hash, &pointer)) { + VIRT_FOREACH(arr_hash, pointer, data) { if (Z_TYPE_P(data) == IS_LONG) { keycodes[i++] = (uint) Z_LVAL_P(data); } - } + } VIRT_FOREACH_END(); if (virDomainSendKey(domain->domain, codeset, holdtime, keycodes, count, flags) != 0) { @@ -5554,14 +5535,8 @@ void parse_array(zval *arr, tVMDisk *disk, tVMNetwork *network) { HashTable *arr_hash; // int array_count; -#if PHP_MAJOR_VERSION >= 7 zval *data; - zend_string *key; -#else - zval **data; // removed **zvalue - char *key; - unsigned int key_len; -#endif + php_libvirt_hash_key_info key; HashPosition pointer; unsigned long index; @@ -5573,83 +5548,41 @@ void parse_array(zval *arr, tVMDisk *disk, tVMNetwork *network) if (network != NULL) memset(network, 0, sizeof(tVMNetwork)); -#if PHP_MAJOR_VERSION >= 7 - for (zend_hash_internal_pointer_reset_ex(arr_hash, &pointer); - (data = zend_hash_get_current_data_ex(arr_hash, &pointer)) != NULL; - zend_hash_move_forward_ex(arr_hash, &pointer)) { + VIRT_FOREACH(arr_hash, pointer, data) { if ((Z_TYPE_P(data) == IS_STRING) || (Z_TYPE_P(data) == IS_LONG)) { - if (zend_hash_get_current_key_ex(arr_hash, &key, &index, &pointer) == HASH_KEY_IS_STRING) { + VIRT_HASH_CURRENT_KEY_INFO(arr_hash, pointer, index, key); + if (key.type == HASH_KEY_IS_STRING) { if (disk != NULL) { - if ((Z_TYPE_P(data) == IS_STRING) && strcmp(ZSTR_VAL(key), "path") == 0) + if ((Z_TYPE_P(data) == IS_STRING) && strcmp(key.name, "path") == 0) disk->path = strdup( Z_STRVAL_P(data) ); - else if ((Z_TYPE_P(data) == IS_STRING) && strcmp(ZSTR_VAL(key), "driver") == 0) + else if ((Z_TYPE_P(data) == IS_STRING) && strcmp(key.name, "driver") == 0) disk->driver = strdup( Z_STRVAL_P(data) ); - else if ((Z_TYPE_P(data) == IS_STRING) && strcmp(ZSTR_VAL(key), "bus") == 0) + else if ((Z_TYPE_P(data) == IS_STRING) && strcmp(key.name, "bus") == 0) disk->bus = strdup( Z_STRVAL_P(data) ); - else if ((Z_TYPE_P(data) == IS_STRING) && strcmp(ZSTR_VAL(key), "dev") == 0) + else if ((Z_TYPE_P(data) == IS_STRING) && strcmp(key.name, "dev") == 0) disk->dev = strdup( Z_STRVAL_P(data) ); - else if (strcmp(ZSTR_VAL(key), "size") == 0) { + else if (strcmp(key.name, "size") == 0) { if (Z_TYPE_P(data) == IS_LONG) { disk->size = Z_LVAL_P(data); } else { disk->size = size_def_to_mbytes(Z_STRVAL_P(data)); } } - else if ((Z_TYPE_P(data) == IS_LONG) && strcmp(ZSTR_VAL(key), "flags") == 0) + else if ((Z_TYPE_P(data) == IS_LONG) && strcmp(key.name, "flags") == 0) disk->flags = Z_LVAL_P(data); } else { if (network != NULL) { - if ((Z_TYPE_P(data) == IS_STRING) && strcmp(ZSTR_VAL(key), "mac") == 0) + if ((Z_TYPE_P(data) == IS_STRING) && strcmp(key.name, "mac") == 0) network->mac = strdup( Z_STRVAL_P(data) ); - else if ((Z_TYPE_P(data) == IS_STRING) && strcmp(ZSTR_VAL(key), "network") == 0) + else if ((Z_TYPE_P(data) == IS_STRING) && strcmp(key.name, "network") == 0) network->network = strdup( Z_STRVAL_P(data) ); - else if ((Z_TYPE_P(data) == IS_STRING) && strcmp(ZSTR_VAL(key), "model") == 0) + else if ((Z_TYPE_P(data) == IS_STRING) && strcmp(key.name, "model") == 0) network->model = strdup( Z_STRVAL_P(data) ); } } } } - } -#else - for (zend_hash_internal_pointer_reset_ex(arr_hash, &pointer); - zend_hash_get_current_data_ex(arr_hash, (void**) &data, &pointer) == SUCCESS; - zend_hash_move_forward_ex(arr_hash, &pointer)) { - if ((Z_TYPE_PP(data) == IS_STRING) || (Z_TYPE_PP(data) == IS_LONG)) { - if (zend_hash_get_current_key_ex(arr_hash, &key, &key_len, &index, 0, &pointer) == HASH_KEY_IS_STRING || HASH_KEY_IS_LONG) { - if (zend_hash_get_current_data_ex(arr_hash, (void**) &data, &pointer) == SUCCESS) { - //DPRINTF("%s: array { key: '%s', val: '%s' }\n", __FUNCTION__, key, Z_STRVAL_PP(data)); - if (disk != NULL) { - if (strcmp(key, "path") == 0) - disk->path = strdup( Z_STRVAL_PP(data) ); - if (strcmp(key, "driver") == 0) - disk->driver = strdup( Z_STRVAL_PP(data) ); - if (strcmp(key, "bus") == 0) - disk->bus = strdup( Z_STRVAL_PP(data) ); - if (strcmp(key, "dev") == 0) - disk->dev = strdup( Z_STRVAL_PP(data) ); - if (strcmp(key, "size") == 0) { - if (Z_TYPE_PP(data) == IS_LONG) - disk->size = Z_LVAL_PP(data); - else - disk->size = size_def_to_mbytes(Z_STRVAL_PP(data)); - } - if (strcmp(key, "flags") == 0) - disk->flags = Z_LVAL_PP(data); - } - else - if (network != NULL) { - if (strcmp(key, "mac") == 0) - network->mac = strdup( Z_STRVAL_PP(data) ); - if (strcmp(key, "network") == 0) - network->network = strdup( Z_STRVAL_PP(data) ); - if (strcmp(key, "model") == 0) - network->model = strdup( Z_STRVAL_PP(data) ); - } - } - } - } - } -#endif + } VIRT_FOREACH_END(); } /* @@ -5691,11 +5624,7 @@ PHP_FUNCTION(libvirt_domain_new) zend_long maxmemMB = -1; HashTable *arr_hash; int numDisks, numNets, i; -#if PHP_MAJOR_VERSION >= 7 zval *data; -#else - zval **data; // removed **zvalue -#endif HashPosition pointer; char vncl[2048] = { 0 }; char tmpname[1024] = { 0 }; @@ -5725,27 +5654,17 @@ PHP_FUNCTION(libvirt_domain_new) vmDisks = (tVMDisk *)malloc( numDisks * sizeof(tVMDisk) ); memset(vmDisks, 0, numDisks * sizeof(tVMDisk)); i = 0; - for(zend_hash_internal_pointer_reset_ex(arr_hash, &pointer); -#if PHP_MAJOR_VERSION >= 7 - (data = zend_hash_get_current_data_ex(arr_hash, &pointer)) != NULL; - zend_hash_move_forward_ex(arr_hash, &pointer)) { + VIRT_FOREACH(arr_hash, pointer, data) { if (Z_TYPE_P(data) == IS_ARRAY) { tVMDisk disk; parse_array(data, &disk, NULL); -#else - zend_hash_get_current_data_ex(arr_hash, (void**) &data, &pointer) == SUCCESS; - zend_hash_move_forward_ex(arr_hash, &pointer)) { - if (Z_TYPE_PP(data) == IS_ARRAY) { - tVMDisk disk; - parse_array(*data, &disk, NULL); -#endif if (disk.path != NULL) { //DPRINTF("Disk => path = '%s', driver = '%s', bus = '%s', dev = '%s', size = %ld MB, flags = %d\n", // disk.path, disk.driver, disk.bus, disk.dev, disk.size, disk.flags); vmDisks[i++] = disk; } } - } + } VIRT_FOREACH_END(); numDisks = i; /* Parse all networks from array */ @@ -5754,26 +5673,16 @@ PHP_FUNCTION(libvirt_domain_new) vmNetworks = (tVMNetwork *)malloc( numNets * sizeof(tVMNetwork) ); memset(vmNetworks, 0, numNets * sizeof(tVMNetwork)); i = 0; - for(zend_hash_internal_pointer_reset_ex(arr_hash, &pointer); -#if PHP_MAJOR_VERSION >= 7 - (data = zend_hash_get_current_data_ex(arr_hash, &pointer)) != NULL; - zend_hash_move_forward_ex(arr_hash, &pointer)) { + VIRT_FOREACH(arr_hash, pointer, data) { if (Z_TYPE_P(data) == IS_ARRAY) { tVMNetwork network; parse_array(data, NULL, &network); -#else - zend_hash_get_current_data_ex(arr_hash, (void**) &data, &pointer) == SUCCESS; - zend_hash_move_forward_ex(arr_hash, &pointer)) { - if (Z_TYPE_PP(data) == IS_ARRAY) { - tVMNetwork network; - parse_array(*data, NULL, &network); -#endif if (network.mac != NULL) { //DPRINTF("Network => mac = '%s', network = '%s', model = '%s'\n", network.mac, network.network, network.model); vmNetworks[i++] = network; } } - } + } VIRT_FOREACH_END(); numNets = i; snprintf(tmpname, sizeof(tmpname), "%s-install", name); -- 2.7.4
On 06.07.2016 23:42, Dawid Zamirski wrote:
With the patches that landed support for PHP7 some API calls were not behaving correctly when compiled against PHP5. The most obvious case is where connection to e.g. esx host, libvirt_connect wasn't properly reading out credentials from Zeng HashTable which resulted in failure to authenticate. This issue was caused mainly be the differnce in Zend's implementaion of zend_hash_get_current_data_ex which in PHP7 takes a pointer to zval whereas in PHP5 it takes pointer to pointer to zval. To address this, the first commit adds macros that abstract away such implementation details and the remaining commits make use of that.
Dawid Zamirski (3): Define macros for looping php hash tables. libvirt_connect: use loop macros to read cred info. use VIRT_FOREACH macros everywhere.
src/libvirt-php.c | 205 +++++++++++++++++------------------------------------- src/libvirt-php.h | 6 ++ 2 files changed, 69 insertions(+), 142 deletions(-)
Thank you, ACKed and pushed. Michal
participants (2)
-
Dawid Zamirski -
Michal Privoznik