lua_add_variable - initialize value (add optional arguments)#107
lua_add_variable - initialize value (add optional arguments)#107krzysin wants to merge 2 commits intoopenresty:masterfrom
Conversation
krzysin
changed the title
|
Hi, Thanks for your PR. Could you please port those changes to https://github.com/openresty/meta-lua-nginx-module as well? The source code in this repository are automatically generated from https://github.com/openresty/meta-lua-nginx-module and editing them here will not persist through next update. |
| if (var->get_handler == NULL) { | ||
| var->get_handler = ngx_stream_lua_undefined_var; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Style: needs a blank line before this line.
|
|
||
| if (var->data) { | ||
| data = (ngx_str_t *) var->data; | ||
| } else { |
src/ngx_stream_lua_directive.c
Outdated
| if (var->data) { | ||
| data = (ngx_str_t *) var->data; | ||
| } else { | ||
| data = ngx_pnalloc(cf->pool, sizeof(ngx_str_t)); |
There was a problem hiding this comment.
Should use ngx_palloc instead since there's only one data chunk to be allocated.
src/ngx_stream_lua_directive.c
Outdated
| } | ||
| data->len = value[2].len; | ||
|
|
||
| data->data = ngx_pnalloc(cf->pool, value[2].len); |
There was a problem hiding this comment.
Ditto. Also, I wonder if the value[2].data memory is also allocated in cf->pool. If yes, then this new allocation and data copying can be saved.
| } | ||
| ngx_memcpy(data->data, value[2].data, value[2].len); | ||
|
|
||
| var->data = (uintptr_t) data; |
There was a problem hiding this comment.
This type casting looks weird. I don't think var->data is of the uintptr_t type at all.
There was a problem hiding this comment.
OK, my bad, it's indeed declared as uintptr_t. Never mind then.
| } | ||
|
|
||
| if (var->data) { | ||
| data = (ngx_str_t *) var->data; |
There was a problem hiding this comment.
I don't think this code path is reacheable. Will you explain?
There was a problem hiding this comment.
This is called when the variable is redefined.
lua_add_variable $ldap_user 'foo';
lua_add_variable $ldap_user 'bar';
src/ngx_stream_lua_directive.c
Outdated
|
|
||
|
|
||
| static ngx_int_t | ||
| ngx_stream_lua_var(ngx_stream_session_t *s, |
There was a problem hiding this comment.
This function name is confusing. I think it should be ngx_stream_lua_var_default_value instead.
Also, better declare this at the beginning of this .c file.
There was a problem hiding this comment.
It's not better that the declarations under ngx_stream_lua_undefined_var ?
src/ngx_stream_lua_directive.c
Outdated
| return NGX_CONF_ERROR; | ||
| } | ||
| } | ||
| data->len = value[2].len; |
There was a problem hiding this comment.
Style: need a newline before this.
| v->not_found = 0; | ||
| v->valid = 1; | ||
| v->no_cacheable = 0; | ||
| v->not_found = 0; |
| v->valid = 1; | ||
| v->no_cacheable = 0; | ||
| v->not_found = 0; | ||
| v->len = str->len; |
There was a problem hiding this comment.
You can just write *v = *str; to make it shorter.
There was a problem hiding this comment.
You can not, v is type ngx_variable_value_t, str is ngx_str_t
src/ngx_stream_lua_directive.c
Outdated
| if (data->data == NULL) { | ||
| return NGX_CONF_ERROR; | ||
| } | ||
| ngx_memcpy(data->data, value[2].data, value[2].len); |
There was a problem hiding this comment.
Style: need a newline before this.
zhuizhuhaomeng
force-pushed
the
master
branch
from
616f118 to
cd24241
Compare
3 participants
No description provided.