Skip to content
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

dnsdist, recursor: EDNSOptionView improvements #7652

Merged
merged 5 commits into from Apr 4, 2019

Conversation

Projects
None yet
2 participants
@Habbie
Copy link
Member

commented Apr 1, 2019

Short description

Doc fix; Lua tables fix (may need upgrade notes?).

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master
@rgacogne

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Sorry about that, I think I will never wrap my head around Lua tables starting at 1. The tests in test_EDNSOptions.py needs to be updated, since the code there relies on the index starting at 0. We have the exact same code in lua-recursor4.cc by the way.

@rgacogne rgacogne added this to the dnsdist-1.4.0 milestone Apr 2, 2019

@Habbie

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Sorry about that, I think I will never wrap my head around Lua tables starting at 1.

I wonder if a simpler vector could be convinced to automatically do the right thing so we have to worry less about it on the C++ side.

The tests in test_EDNSOptions.py needs to be updated, since the code there relies on the index starting at 0.

I suspected that was the reason of the test failures :)

We have the exact same code in lua-recursor4.cc by the way.

Thanks!

Habbie added some commits Apr 2, 2019

@Habbie

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

I wonder if a simpler vector could be convinced to automatically do the right thing so we have to worry less about it on the C++ side.

Looks like we can - see commit 'simplify vector indexing'. As discussed elsewhere, we wonder why we didn't do that before.

@Habbie Habbie changed the title dnsdist: EDNSOptionView improvements dnsdist, recursor: EDNSOptionView improvements Apr 2, 2019

@Habbie

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

For future reference: to find accidental 0-indexing, apply this patch, then run all tests, see what fails, then weed out false negatives. I cannot think of a good way to apply this to our usual testing.

diff --git a/ext/luawrapper/include/LuaContext.hpp b/ext/luawrapper/include/LuaContext.hpp
index b4c9ef15e..124349eaf 100644
--- a/ext/luawrapper/include/LuaContext.hpp
+++ b/ext/luawrapper/include/LuaContext.hpp
@@ -2137,6 +2137,56 @@ struct LuaContext::Pusher<std::unordered_map<TKey,TValue,THash,TKeyEqual>> {
     }
 };
 
+
+
+
+// vectors of int, X pairs
+template<typename TType2>
+struct LuaContext::Pusher<std::vector<std::pair<int,TType2>>> {
+    static const int minSize = 1;
+    static const int maxSize = 1;
+
+    static PushedObject push(lua_State* state, const std::vector<std::pair<int,TType2>>& value) noexcept {
+        static_assert(Pusher<typename std::decay<int>::type>::minSize == 1 && Pusher<typename std::decay<int>::type>::maxSize == 1, "Can't push multiple elements for a table key");
+        static_assert(Pusher<typename std::decay<TType2>::type>::minSize == 1 && Pusher<typename std::decay<TType2>::type>::maxSize == 1, "Can't push multiple elements for a table value");
+
+        auto obj = Pusher<EmptyArray_t>::push(state, EmptyArray);
+
+        if(value.size() && value.begin()->first == 0) {
+            throw std::runtime_error("0-indexed vector spotted");
+        }
+
+        for (auto i = value.begin(), e = value.end(); i != e; ++i)
+            setTable<TType2>(state, obj, i->first, i->second);
+        
+        return obj;
+    }
+};
+
+// vectors of short, X pairs
+template<typename TType2>
+struct LuaContext::Pusher<std::vector<std::pair<short,TType2>>> {
+    static const int minSize = 1;
+    static const int maxSize = 1;
+
+    static PushedObject push(lua_State* state, const std::vector<std::pair<short,TType2>>& value) noexcept {
+        static_assert(Pusher<typename std::decay<short>::type>::minSize == 1 && Pusher<typename std::decay<short>::type>::maxSize == 1, "Can't push multiple elements for a table key");
+        static_assert(Pusher<typename std::decay<TType2>::type>::minSize == 1 && Pusher<typename std::decay<TType2>::type>::maxSize == 1, "Can't push multiple elements for a table value");
+
+        auto obj = Pusher<EmptyArray_t>::push(state, EmptyArray);
+
+        if(value.size() && value.begin()->first == 0) {
+            throw std::runtime_error("0-indexed vector spotted");
+        }
+
+        for (auto i = value.begin(), e = value.end(); i != e; ++i)
+            setTable<TType2>(state, obj, i->first, i->second);
+        
+        return obj;
+    }
+};
+
+
 // vectors of pairs
 template<typename TType1, typename TType2>
 struct LuaContext::Pusher<std::vector<std::pair<TType1,TType2>>> {

@Habbie Habbie added the rec label Apr 2, 2019

@Habbie

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

We have the exact same code in lua-recursor4.cc by the way.

Also fixed now.

@rgacogne rgacogne merged commit e7ce60e into PowerDNS:master Apr 4, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
ci/circleci: build Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.