Re: com php-src: avoid multiple strlen calls for the same buffer: Zend/zend_virtual_cwd.c

On Fri, September 19, 2014 12:57, Nikita Popov wrote: > On Fri, Sep 19, 2014 at 12:39 PM, Anatol Belski <ab@php.net> wrote: > > >> Commit: 6bbebc60ea0de6ce09ea45094b3bed1823d96cec >> Author: Anatol Belski <ab@php.net> Fri, 19 Sep 2014 12:39:17 >> +0200 >> Parents: d8de53d498cf75936c9cd6781456c39b01f5af60 >> Branches: master >> >> >> Link: >> >> http://git.php.net/?p=php-src.git;a=commitdiff;h=6bbebc60ea0de6ce09ea450 >> 94b3bed1823d96cec >> >> >> Log: >> avoid multiple strlen calls for the same buffer >> >> Changed paths: >> M Zend/zend_virtual_cwd.c >> >> >> >> Diff: >> diff --git a/Zend/zend_virtual_cwd.c b/Zend/zend_virtual_cwd.c index >> 665829d..acc83ec 100644 >> --- a/Zend/zend_virtual_cwd.c >> +++ b/Zend/zend_virtual_cwd.c >> @@ -1442,16 +1442,20 @@ CWD_API char *virtual_realpath(const char *path, >> char *real_path TSRMLS_DC) /* { if (VCWD_GETCWD(cwd, MAXPATHLEN)) { path >> = cwd; >> } >> - } else if (!IS_ABSOLUTE_PATH(path, strlen(path))) { >> - CWD_STATE_COPY(&new_state, &CWDG(cwd)); >> } else { >> - new_state.cwd = (char*)emalloc(1); >> - if (new_state.cwd == NULL) { >> - retval = NULL; >> - goto end; >> + size_t path_len = strlen(path); >> + >> + if (!IS_ABSOLUTE_PATH(path, path_len)) { >> + CWD_STATE_COPY(&new_state, &CWDG(cwd)); >> + } else { >> + new_state.cwd = (char*)emalloc(1); >> + if (new_state.cwd == NULL) { >> + retval = NULL; >> + goto end; >> + } >> + new_state.cwd[0] = '\0'; >> + new_state.cwd_length = 0; >> } >> - new_state.cwd[0] = '\0'; >> - new_state.cwd_length = 0; >> } >> >> >> if (virtual_file_ex(&new_state, path, NULL, CWD_REALPATH TSRMLS_CC)==0) >> { >> @@ -1967,17 +1971,21 @@ CWD_API char *tsrm_realpath(const char *path, >> char *real_path TSRMLS_DC) /* {{{ >> if (VCWD_GETCWD(cwd, MAXPATHLEN)) { path = cwd; } >> - } else if (!IS_ABSOLUTE_PATH(path, strlen(path)) && >> - VCWD_GETCWD(cwd, MAXPATHLEN)) { >> - new_state.cwd = estrdup(cwd); >> - new_state.cwd_length = strlen(cwd); >> } else { >> - new_state.cwd = (char*)emalloc(1); >> - if (new_state.cwd == NULL) { >> - return NULL; >> + size_t path_len = strlen(path); >> + >> + if (!IS_ABSOLUTE_PATH(path, strlen(path)) && >> + VCWD_GETCWD(cwd, MAXPATHLEN)) { >> + new_state.cwd = estrdup(cwd); >> + new_state.cwd_length = strlen(cwd); >> + } else { >> + new_state.cwd = (char*)emalloc(1); >> + if (new_state.cwd == NULL) { >> + return NULL; >> + } >> + new_state.cwd[0] = '\0'; >> + new_state.cwd_length = 0; >> } >> - new_state.cwd[0] = '\0'; >> - new_state.cwd_length = 0; >> } >> >> >> if (virtual_file_ex(&new_state, path, NULL, CWD_REALPATH TSRMLS_CC)) { >> >> > > In the second case, you forgot to replace the strlen(path) in the > IS_ABSOLUTE_PATH invocation. > > > Also, these changes will cause unused variable warnings on non-windows > systems, because IS_ABSOLUTE_PATH only uses the length on windows. I'm not > sure what's the best way to avoid that - maybe make IS_ABSOLUTE_PATH an > inline function instead of a macro? > Ah, you're right, those will be warned as unreferenced, and even if not - that's an empty call. A grep reports like 34 usage occurrences in the core, not that many. Those I've touched was using strlen(), are about 5 AFAIR. In the other cases the path length is delivered. Though, a couple of places can be optimized. I'll try with the inline function, it's cleaner. If it takes too much, will just go through the places I've touched and #ifdef them. Those macros are normally not changed/used that often, so should be ok, too. Regards Anatol