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.

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

rec: Add a new Lua FFI hook, gettag_ffi #6344

Merged
merged 4 commits into from Mar 23, 2018

Conversation

Projects
None yet
3 participants
@rgacogne
Member

rgacogne commented Mar 14, 2018

Short description

Add a new gettag_ffi() hook, which is an FFI-friendly version of the existing gettag().

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

rgacogne added some commits Mar 12, 2018

@rgacogne rgacogne added this to the auth-4.1.x milestone Mar 14, 2018

@rgacogne rgacogne changed the title from rec: Add a new Lua FFI hook, gettag-ffi to rec: Add a new Lua FFI hook, gettag_ffi Mar 14, 2018

@Habbie

This comment has been minimized.

Member

Habbie commented Mar 15, 2018

There was something about linker flags, -rdynamic or something?

@Habbie Habbie closed this Mar 15, 2018

@Habbie Habbie reopened this Mar 15, 2018

@rgacogne

This comment has been minimized.

Member

rgacogne commented Mar 15, 2018

Right, so it looks like I don't have the issue because I have SNMP support enabled, meaning I inherit -Wl,-E from NET_SNMP_LIBS. I guess we should add that flag to LDFLAGS as soon as LuaJIT support is enabled?

@rgacogne

This comment has been minimized.

Member

rgacogne commented Mar 15, 2018

After reading 1 I would be more inclined to just add __attribute__ ((visibility ("default"))) to the few FFI functions we need, which would be a small change:

diff --git a/pdns/lua-recursor4-ffi.hh b/pdns/lua-recursor4-ffi.hh
index 64e5a433a..7fa08f78f 100644
--- a/pdns/lua-recursor4-ffi.hh
+++ b/pdns/lua-recursor4-ffi.hh
@@ -29,24 +29,24 @@ extern "C" {
     const void* data;
   } pdns_ednsoption_t;
 
-  const char* pdns_ffi_param_get_qname(pdns_ffi_param_t* ref);
-  uint16_t pdns_ffi_param_get_qtype(const pdns_ffi_param_t* ref);
-  const char* pdns_ffi_param_get_remote(pdns_ffi_param_t* ref);
-  uint16_t pdns_ffi_param_get_remote_port(const pdns_ffi_param_t* ref);
-  const char* pdns_ffi_param_get_local(pdns_ffi_param_t* ref);
-  uint16_t pdns_ffi_param_get_local_port(const pdns_ffi_param_t* ref);
-  const char* pdns_ffi_param_get_edns_cs(pdns_ffi_param_t* ref);
-  uint8_t pdns_ffi_param_get_edns_cs_source_mask(const pdns_ffi_param_t* ref);
+  const char* pdns_ffi_param_get_qname(pdns_ffi_param_t* ref) __attribute__ ((visibility ("default")));
+  uint16_t pdns_ffi_param_get_qtype(const pdns_ffi_param_t* ref) __attribute__ ((visibility ("default")));
+  const char* pdns_ffi_param_get_remote(pdns_ffi_param_t* ref) __attribute__ ((visibility ("default")));
+  uint16_t pdns_ffi_param_get_remote_port(const pdns_ffi_param_t* ref) __attribute__ ((visibility ("default")));
+  const char* pdns_ffi_param_get_local(pdns_ffi_param_t* ref) __attribute__ ((visibility ("default")));
+  uint16_t pdns_ffi_param_get_local_port(const pdns_ffi_param_t* ref) __attribute__ ((visibility ("default")));
+  const char* pdns_ffi_param_get_edns_cs(pdns_ffi_param_t* ref) __attribute__ ((visibility ("default")));
+  uint8_t pdns_ffi_param_get_edns_cs_source_mask(const pdns_ffi_param_t* ref) __attribute__ ((visibility ("default")));
 
   // returns the length of the resulting 'out' array. 'out' is not set if the length is 0
-  size_t pdns_ffi_param_get_edns_options(pdns_ffi_param_t* ref, const pdns_ednsoption_t** out);
-  size_t pdns_ffi_param_get_edns_options_by_code(pdns_ffi_param_t* ref, uint16_t optionCode, const pdns_ednsoption_t** out);
+  size_t pdns_ffi_param_get_edns_options(pdns_ffi_param_t* ref, const pdns_ednsoption_t** out) __attribute__ ((visibility ("default")));
+  size_t pdns_ffi_param_get_edns_options_by_code(pdns_ffi_param_t* ref, uint16_t optionCode, const pdns_ednsoption_t** out) __attribute__ ((visibility ("default")));
 
-  void pdns_ffi_param_set_tag(pdns_ffi_param_t* ref, unsigned int tag);
-  void pdns_ffi_param_add_policytag(pdns_ffi_param_t *ref, const char* name);
-  void pdns_ffi_param_set_requestorid(pdns_ffi_param_t* ref, const char* name);
-  void pdns_ffi_param_set_devicename(pdns_ffi_param_t* ref, const char* name);
-  void pdns_ffi_param_set_deviceid(pdns_ffi_param_t* ref, size_t len, const void* name);
-  void pdns_ffi_param_set_variable(pdns_ffi_param_t* ref, bool variable);
-  void pdns_ffi_param_set_ttl_cap(pdns_ffi_param_t* ref, uint32_t ttl);
+  void pdns_ffi_param_set_tag(pdns_ffi_param_t* ref, unsigned int tag) __attribute__ ((visibility ("default")));
+  void pdns_ffi_param_add_policytag(pdns_ffi_param_t *ref, const char* name) __attribute__ ((visibility ("default")));
+  void pdns_ffi_param_set_requestorid(pdns_ffi_param_t* ref, const char* name) __attribute__ ((visibility ("default")));
+  void pdns_ffi_param_set_devicename(pdns_ffi_param_t* ref, const char* name) __attribute__ ((visibility ("default")));
+  void pdns_ffi_param_set_deviceid(pdns_ffi_param_t* ref, size_t len, const void* name) __attribute__ ((visibility ("default")));
+  void pdns_ffi_param_set_variable(pdns_ffi_param_t* ref, bool variable) __attribute__ ((visibility ("default")));
+  void pdns_ffi_param_set_ttl_cap(pdns_ffi_param_t* ref, uint32_t ttl) __attribute__ ((visibility ("default")));
 }
rec: Set the visibility of FFI functions to 'default' (external)
This makes the symbols usable across shared object boundary (Lua FFI)
even if the default visibility is set to hidden.
@rgacogne

This comment has been minimized.

Member

rgacogne commented Mar 16, 2018

Pushed a commit adding __attribute__ ((visibility ("default"))).

@@ -1729,7 +1743,7 @@ static string* doProcessUDPQuestion(const std::string& question, const ComboAddr
*/
#endif
if(needECS || needXPF || (t_pdl && t_pdl->d_gettag)) {
if(needECS || needXPF || (t_pdl && (t_pdl->d_gettag || t_pdl->d_gettag_ffi))) {

This comment has been minimized.

@pieterlexis

pieterlexis Mar 22, 2018

Member

on line 1555 we do the gettag in a different order. If for some reason both gettag and gettag_ffi are defined, this might lead to inconsistent behaviour.

This comment has been minimized.

@rgacogne

rgacogne Mar 22, 2018

Member

We are just looking to see if at least one of them is defined here, so I don't think it matters? The actual invocation is done on lines 1568 and 1762, and we do consistently look for the FFI version first there.

@pieterlexis

I can't comment on the Lua bits, but I have one comment on the code and would like to see tests (also for gettag... but that is out of scope for this PR).

@pieterlexis pieterlexis merged commit c298c4d into PowerDNS:master Mar 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rgacogne rgacogne deleted the rgacogne:rec-lua-ffi-clean branch Mar 23, 2018

@rgacogne rgacogne modified the milestones: auth-4.1.x, rec-4.1.x Apr 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment