Update remove_reserved_networks option to exclude these networks from the in-memory tree #90

horgh · 2017-10-19T23:54:27Z

The intention is to make it so we cannot add these networks and then see them when working with the in-memory representation of the tree.

…y records This adds a new record type for this purpose: A fixed empty record. These are like empty records in that they indicate there is no information about the networks, but they are also immutable and you cannot add networks under them (the latter being similar to alias records). We now add reserved networks with this record type. This means it will not be possible to inadvertently add such networks at all when the option to remove them is on. Previously that option worked by removing such networks from the tree after the fact. This was problematic if you used the tree prior to that point.

I noticed an unused field, and a possibly missing else case. Also give more information about record types.

And clarify the types we get there. This also allows removing an unused error code.

Several do not expect the reserved networks to be in the tree, even empty. For example, those that iterate, and those that freeze/thaw.

This is necessary for the cases where we want an identical after thawing when we freeze a tree with this option.

Previously we were treating it as a data record which was not correct

Due to bugs fixed in the last two commits we were turning off this option more than was needed. That fixed tests but hit the bugs. I now comment where it is needed to be disabled and why.

This leads to an error depending on the order we try to insert nodes. If we try to insert a less specific fixed node after we already have fixed nodes, without this we will insert that node into multiple spots, leading to a segfault.

oschwald

I mostly had comments. There was one small change that I think we need where we copy a value from a fixed empty.

I am a bit worried about the behavior change when inserting into a reserved network. We would need to add checks to our internal code to account for this. I should have thought of this and raised it earlier.

oschwald · 2017-10-20T22:16:30Z

c/tree.c

+    if (current_record->type != MMDBW_RECORD_TYPE_EMPTY &&
+        current_record->type != MMDBW_RECORD_TYPE_DATA &&
+        current_record->type != MMDBW_RECORD_TYPE_NODE) {
+        croak(


I've been trying to move away from using croaks in this code as it makes it even harder to keep track of what needs to be freed or cleaned up. However, based on the description, this should only happen if there is a programming error in the writer, which seems fine.

I see, yeah that makes sense. Yes, this one should probably not happen in normal use either, although I think it would be easy to just use an error status. I'll take a look at it and others I added.

oschwald · 2017-10-20T22:17:54Z

c/tree.c

    tree->merge_cache = NULL;
    tree->data_table = NULL;
+    // TODO(wstorey@maxmind.com): What is the purpose of this? It appears
+    // unused.


I suspect you are right. I think it is leftover from before #81.

Cool, I'll remove it!

oschwald · 2017-10-20T22:23:05Z

c/tree.c

+        free_network(&resolved_network);
+
+        if (MMDBW_SUCCESS != status) {
+            croak(


We would leak memory if this happened. However, it seems like it would only happen on a programming error in the writer code.

See comment on croak below.

Yeah, this one we should be pretty safe on not happening

oschwald · 2017-10-20T22:25:29Z

c/tree.c

+        new_record,
+        merge_strategy,
+        is_internal_insert
+        );


Is the indentation change from running uncrustify?

Yeah. Can't say I'm a fan

oschwald · 2017-10-20T22:33:56Z

c/tree.c

            }
+            // current_bit < network->prefix_length. This means we contain the
+            // new network already as FIXED_EMPTY/ALIAS. Inserting the network
+            // is not valid because of that.


One concern I have about making this behave the same was as alias nodes--which I realize I suggested--is that it is a fairly significant behavior change and we will now throw an exception when inserting into a private network. We will have to add checks to the GeoIP2 build to ensure that we don't do this. Another option that is more similar to the old behavior is to just silently drop the data. I am not sure I love that either though.

Yeah that's a good point. We do silently drop the inserts already if they contain the network, as you can see, so doing so for into/replacement would probably be okay and would not be too difficult. It would add some consistency, arguably.

I wonder if we should add an option to be strict vs not. But I think we should probably not complicate this code by adding toggles if possible.

One argument in favour of having errors might be that it could lead to cleaning up data by the callers, but I don't know how much work that would generate, or if it would be even valuable.

An argument in favour of being silent is that was the prior behaviour!

I'm open to either way. Not sure which to go with. We could perhaps raise it with someone involved in the build and see what they think.

oschwald · 2017-10-20T22:35:24Z

c/tree.c

                                                 merge_strategy
                                                 );

+    // TODO(wstorey@maxmind.com): Should we check success?


oschwald · 2017-10-20T22:37:06Z

c/tree.c

        current_record->value.node = new_record->value.node;
    }
+    // TODO(wstorey@maxmind.com): Should we have an else? What about other
+    // record types? EMPTY isn't dealt with here.


Actually, I think we should remove MMDBW_RECORD_TYPE_FIXED_EMPTY above. ->value is not set on empty nodes.

Ah good point. I was wondering about whether we should zero it in the case of EMPTY mainly. Although I don't know that we even get new_record->type being empty here actually... But we would FIXED_EMPTY of course.

oschwald · 2017-10-20T22:38:34Z

c/tree.c

-
-    return hex;
-}
-


Thanks for cleaning this stuff up!

It's pretty redundant and not entirely accurate

This still croaks in new_tree(), but that is pre-existing, and showing some kind of reason from there makes sense I think. Possibly we could switch to warning and return NULL.

These don't have values.

This means we will no longer throw exceptions. This is to behave as we did with the remove_reserved_networks option prior to beginning this work.

horgh added 23 commits October 18, 2017 09:13

Remove dead code generating the IP as a string

f6fbf25

Fix typo in subtest name

49e6aee

Add comments

eb97655

I noticed an unused field, and a possibly missing else case. Also give more information about record types.

Remove dead code from insert_record_into_current_record

c3dc5ca

And clarify the types we get there. This also allows removing an unused error code.

Remove an unused error code

1a5fd35

Simplify an elseif/else block that uses early returns

a1fa17a

Remove several unused functions

28af3af

Update remaining existing tests so they pass again

2e92a70

Several do not expect the reserved networks to be in the tree, even empty. For example, those that iterate, and those that freeze/thaw.

Pass through the remove_reserved_networks when thawing

ddfd74a

This is necessary for the cases where we want an identical after thawing when we freeze a tree with this option.

Recognize FIXED_EMPTY as empty when iterating

d00b43b

Previously we were treating it as a data record which was not correct

Update tests to not remove reserved networks unless it's necessary

5dfcad4

Due to bugs fixed in the last two commits we were turning off this option more than was needed. That fixed tests but hit the bugs. I now comment where it is needed to be disabled and why.

Treat FIXED_EMPTY like EMPTY in one more spot

e4e844f

Add a couple tests and comment some tests

9f4819d

Clarify reserved networks and comment the edge cases

bddc9f9

Add new tests

56a7c99

Run uncrustify

48b54f6

Fix memory leak from fixed empty records

9df4df4

Update a couple comments

32fe319

Run tidyall

abd827c

Update changelog

5888467

Remove unused parameter causing warning

7ebc7a1

oschwald suggested changes Oct 20, 2017

View reviewed changes

horgh added 6 commits October 24, 2017 09:43

Remove unused field tree->is_aliased

ae68956

Remove a comment

299d0b4

It's pretty redundant and not entirely accurate

Add a missing ) in a comment

98e67a2

Return status rather than croaking when inserting reserved networks

88cf8f5

This still croaks in new_tree(), but that is pre-existing, and showing some kind of reason from there makes sense I think. Possibly we could switch to warning and return NULL.

Remove a croak from insert_record_into_current_record()

df07498

Check success of freeing a record when replacing it

52b88ec

horgh added 2 commits October 24, 2017 10:54

Set value to NULL when setting a record to an EMPTY/FIXED_EMPTY

13ebb56

These don't have values.

Silently ignore inserts to reserved networks

a1d0452

This means we will no longer throw exceptions. This is to behave as we did with the remove_reserved_networks option prior to beginning this work.

oschwald merged commit 42a5e5a into master Oct 25, 2017

oschwald deleted the horgh/fixed-reserved-networks branch October 25, 2017 17:02

Update remove_reserved_networks option to exclude these networks from the in-memory tree #90

Update remove_reserved_networks option to exclude these networks from the in-memory tree #90

Uh oh!

Conversation

horgh commented Oct 19, 2017

Uh oh!

oschwald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants