Skip to content

feature: exposed ngx_http_lua_ffi_str_t for other Nginx modules. #1273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
spacewander wants to merge 1 commit into from
Closed

feature: exposed ngx_http_lua_ffi_str_t for other Nginx modules. #1273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,7 @@ HTTP_LUA_DEPS=" \
$ngx_addon_dir/src/ngx_http_lua_shdict.h \
$ngx_addon_dir/src/ngx_http_lua_socket_tcp.h \
$ngx_addon_dir/src/api/ngx_http_lua_api.h \
$ngx_addon_dir/src/api/ngx_http_lua_api_common.h \
$ngx_addon_dir/src/ngx_http_lua_logby.h \
$ngx_addon_dir/src/ngx_http_lua_sleep.h \
$ngx_addon_dir/src/ngx_http_lua_semaphore.h\
Expand Down
1 change: 1 addition & 0 deletions src/api/ngx_http_lua_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <lua.h>
#include <stdint.h>

#include "ngx_http_lua_api_common.h"
Copy link
Copy Markdown
Member

@agentzh agentzh Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's wrong to include common.h in api.h since it exposes all ngx_lua's internal types and symbols, which is not desired.

Copy link
Copy Markdown
Member Author

@spacewander spacewander Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agentzh
No, ngx_http_lua_api_common.h is not ngx_http_lua_common.h.

Copy link
Copy Markdown
Member

@agentzh agentzh Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacewander Why introduce this new common.h file? It's confusing ;) Let's simply put everything inside this api.h. This file is small anyway.

Copy link
Copy Markdown
Member Author

@spacewander spacewander Mar 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agentzh
The ngx_http_lua_ffi_str_t is required inside of the lua-nginx-module too. Put it into api.h will require the other headers to include the public API header. I think it could be more flexible by adding a middle layer.

Copy link
Copy Markdown
Member

@agentzh agentzh Mar 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacewander Hmm, I disagree here. Internally we should use ngx_str_t directly.

Copy link
Copy Markdown
Member Author

@spacewander spacewander Mar 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agentzh
ngx_http_lua_ffi_str_t is used in ngx_http_lua_util.h:
https://github.com/openresty/lua-nginx-module/blob/master/src/ngx_http_lua_util.h#L25

If we put ngx_http_lua_ffi_str_t into api.h, then we should include the public API header into the internal utility header, which is strange.

Copy link
Copy Markdown
Member

@agentzh agentzh Mar 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacewander I don't want that api_common.h file. It's horrible and confusing. You can include the whole api.h file if necessary. I don't mind.


/* Public API for other Nginx modules */

Expand Down
19 changes: 19 additions & 0 deletions src/api/ngx_http_lua_api_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@

#ifndef _NGX_HTTP_LUA_API_COMMON_H_INCLUDED_
#define _NGX_HTTP_LUA_API_COMMON_H_INCLUDED_

#include <ngx_core.h>


#ifndef NGX_LUA_NO_FFI_API
typedef struct {
int len;
/* this padding hole on 64-bit systems is expected */
u_char *data;
} ngx_http_lua_ffi_str_t;
#endif /* NGX_LUA_NO_FFI_API */


#endif /* _NGX_HTTP_LUA_API_COMMON_H_INCLUDED_ */

/* vi:set ft=c ts=4 sw=4 et fdm=marker: */
2 changes: 2 additions & 0 deletions src/ngx_http_lua_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
#include <lualib.h>
#include <lauxlib.h>

#include "api/ngx_http_lua_api_common.h"


#if (NGX_PCRE)

Expand Down
7 changes: 0 additions & 7 deletions src/ngx_http_lua_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,6 @@


#ifndef NGX_LUA_NO_FFI_API
typedef struct {
int len;
/* this padding hole on 64-bit systems is expected */
u_char *data;
} ngx_http_lua_ffi_str_t;


typedef struct {
ngx_http_lua_ffi_str_t key;
ngx_http_lua_ffi_str_t value;
Expand Down