Skip to content

Conversation

moteus
Copy link
Contributor

@moteus moteus commented Dec 28, 2016

I only add safe way to handle invalid input.
Usage

local db, err = mmdb.open_safe("GeoLite2-City.mmdb")
assert(db, err)
local info, err = db:search_ipv4('192.168.123.1')
if err  then -- error 
if not info then -- item not found

Not sure is it worth convert also errors based on invalid database.
This list of rest errors

assert(length == 8, "double of non-8 length") 
assert(length == 8, "double of non-8 length")
assert(type(key) == "string") 
error("Unknown data section: " .. data_type)
error("Cyclical tree") 

I think it should be converted too.
Only problem it requires few more if for each call read_data

Copy link
Owner

@daurnimator daurnimator left a comment

Choose a reason for hiding this comment

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

Note there is also the asserts about float length; and I think there are a few places I was relying on checking built into string.unpack?

mmdb/init.lua Outdated
node = node or 0
local seen = { [node] = true }
for _, direction in ipairs(bits) do
for i = 1, #bits do
Copy link
Owner

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory it more faster way to do iteration. So I just prefer this one.
I saw this in several places. e.g https://springrts.com/wiki/Lua_Performance#TEST_9:_for-loops.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, but there is a slight change in behaviour: ipairs always stops at first nil; while your code is undefined in the presence of nils.

I'm happy to make the change, but please include it in a different commit (should possibly even be in a different PR, but I suppose we can sneak it in)

mmdb/init.lua Outdated
local function ipv4_to_bit_array(str)
local o1, o2, o3, o4 = str:match("(%d%d?%d?)%.(%d%d?%d?)%.(%d%d?%d?)%.(%d%d?%d?)")
assert(o1, "invalid IPv4 address")
if not o1 then return "invalid IPv4 address" end
Copy link
Owner

Choose a reason for hiding this comment

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

please put return on it's own line

mmdb/init.lua Outdated
n = n + 1
u16 = tonumber(u16, 16)
assert(u16, "invalid IPv6 address")
if not u16 then return nil, "invalid IPv6 address" end
Copy link
Owner

Choose a reason for hiding this comment

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

please put return on it's own line

mmdb/init.lua Outdated
fd:close()
assert(contents, err)

if not contents then return fail(safe, err) end
Copy link
Owner

Choose a reason for hiding this comment

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

The changes in open_db are already reflected in #10

mmdb/init.lua Outdated
return self
end

function geodb_methods:fail(...)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't want to expose a :fail method as part of the public api

Update. Do not export `fail` function
Update. Code style.
Copy link
Owner

@daurnimator daurnimator left a comment

Choose a reason for hiding this comment

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

Still have more returns that need to be on their own lines.
Also consider rebasing on top of #10. However could you review my question over there?

mmdb/init.lua Outdated
node = node or 0
local seen = { [node] = true }
for _, direction in ipairs(bits) do
for i = 1, #bits do
Copy link
Owner

Choose a reason for hiding this comment

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

Okay, but there is a slight change in behaviour: ipairs always stops at first nil; while your code is undefined in the presence of nils.

I'm happy to make the change, but please include it in a different commit (should possibly even be in a different PR, but I suppose we can sneak it in)

mmdb/init.lua Outdated
local function open_db(filename)
local fd = assert(io.open(filename, "rb"))
local function fail(safe, ...)
if safe then return nil, ... end
Copy link
Owner

Choose a reason for hiding this comment

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

return on own line

mmdb/init.lua Outdated

return {
open = open_db;
open = function(...) return open_db(false, ...) end;
Copy link
Owner

Choose a reason for hiding this comment

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

Please just declare these in the body of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be use this one ?

local mmdb = { here you can add e.g. version info, module name }

function mmdb.open() ...end

function mmdb.open_safe() ...end

return mmdb

return select(2, self:search(ipv4_to_bit_array(str), self.ipv4_start))
local bits, err = ipv4_to_bit_array(str)
if not bits then
return fail(self.safe, err)
Copy link
Owner

Choose a reason for hiding this comment

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

No need to call fail here: can return bits, err to the caller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants