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

Limit on excluding domains from top lists #471

Closed
3 tasks done
WaLLy3K opened this issue Apr 12, 2017 · 10 comments
Closed
3 tasks done

Limit on excluding domains from top lists #471

WaLLy3K opened this issue Apr 12, 2017 · 10 comments

Comments

@WaLLy3K
Copy link
Contributor

WaLLy3K commented Apr 12, 2017

In raising this issue, I confirm the following:

How familiar are you with the codebase?:

4


[BUG] Expected Behaviour:
If putting in a large amount (50+) of domains within "Top Domains / Top Advertisers" (and possibly "Top Clients"), one expects to have domains removed from the Top Domains list on the dashboard.

[BUG] Actual Behaviour:
Domains after the 50th entry appear to be ignored, and are still listed on the Top Domains list on the dashboard

[BUG] Steps to reproduce:
Place 50 domains within the list found in Settings > API > Top Domains / Top Advertisers, and see whether domains after 50 are removed from the dashboard.

Debug token generated by pihole -d:
h84i2r8laq

@AzureMarker
Copy link
Contributor

Very odd, I can't reproduce on dev, so it might be fixed in the next update. Here's the relevant code on master, which doesn't appear to be limiting the amount of domains you can remove.

@WaLLy3K
Copy link
Contributor Author

WaLLy3K commented Apr 14, 2017

This issue is present on the latest dev version on both my 3B running DietPi, and my Zero running RJL. With the Zero, I could reproduce the issue by copying my list over from the Admin Console and I could confirm the issue further by placing a domain from the bottom of the Top Domains API (that was showing on the dashboard Top Domains list) to the top.

Maybe it's a formatting issue that's being pasted across?

@AzureMarker
Copy link
Contributor

Perhaps it's a formatting issue. Have you tried restarting FTL after making the change? Perhaps it's a bug in FTL, since that is what provides all the stats. I did see in your debug log that there were 51 excluded domains; is that how many you entered?

@WaLLy3K
Copy link
Contributor Author

WaLLy3K commented Apr 14, 2017

FTL's been restarted along with the box numerous times, and yeah, I've excluded 51 domains (which just to confirm we're on the same page, is the entire point)

@DL6ER DL6ER self-assigned this Apr 14, 2017
@DL6ER
Copy link
Member

DL6ER commented Apr 14, 2017

Now that I think about it again, I remember that I explicitly assumed that single lines in the setupVars.conf file are not longer than 1KB. Without looking into the code, I assume that you simply hit this limit. I'll increase the limit to something larger (maybe 100KB). Having said that I'm not in favor of a dynamical solution for the buffer size.

@WaLLy3K
Copy link
Contributor Author

WaLLy3K commented Apr 14, 2017

That sounds like it could be the case! I previously thought I hit a limit at around 47, but I removed a couple of domains and found I was able to add up to 50.

Assuming that 1KB is 50 domains, 100KB would eliminate any need to have a dynamic solution.

@DL6ER
Copy link
Member

DL6ER commented Apr 14, 2017

Try

awk '{++a[length()]} END{for (i in a) print i, a[i]}' /etc/pihole/setupVars.conf

To get the length of all lines in the config file and look for a line that is close to/longer than 1023

@WaLLy3K
Copy link
Contributor Author

WaLLy3K commented Apr 14, 2017

1075 1
11 1
12 1
20 1
13 1
21 2
22 2
23 1
16 1
24 1
18 1
26 1
28 1
37 1
0 2

@DL6ER
Copy link
Member

DL6ER commented Apr 14, 2017

There we are, one line with length 1075 has been found.

@DL6ER
Copy link
Member

DL6ER commented Apr 14, 2017

Fixed in pi-hole/FTL@055ce53

Released as FTL v2.3.1

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

No branches or pull requests

3 participants