Reviewers for a new package

Hi folks

Recently I sent a pull request to the packages repo to add my package (geoip-shell). So today, with generous help from @robimarko, it passed all checks. To my understanding, at this point I should look for people to review it.

What I still don't know is what is the criteria for reviewers? Do they need to do code review, functional review, either or both?

The PR is here, in case someone wants to give it a hand :slight_smile:

https://github.com/openwrt/packages/pull/23821

The code is pulled by the build system from this repo (comments are stripped from the code to reduce its size):

https://github.com/friendly-bits/geoip-shell-openwrt

Project's main repo has the code with comments:

https://github.com/friendly-bits/geoip-shell

The main repo has scripts which automate packaging (prep-owrt-package.sh and mk-owrt-package.sh, in OpenWrt/ subdirectory). The prep- script prepares the code for packaging, the mk- script employs the build system to package and compile it.

P.s. I'm not a professional coder but I've put a lot of work into this project. It's most likely not perfect but I'm prepared to fix any issues a (qualified) reviewer finds in it. I'm using it on my router and on my server, so far neither blew up :slight_smile:

1 Like

@brada4 I've seen your comment on Github

  • add random sleep in crontab script for like half of period.

Your idea is to prevent situation where everyone updates at the same time? If so, I can do it, no problem. I'll implement this in the -run script though, rather than in the crontab command.

As to the "points to consider in future devlopment", I understood very little of it :slight_smile:

  • make independent hook right before raw/prerouting to save slab allocations for new states thus using less resources dropping same packet also blocking out existing connections on change
  • same but deleting conntrack states that fall in blacklist with conntrack command if such is installed

What do you mean by 'make independent hook'? I can't make netfilter hooks, I can only use existing hooks... Can you give an example nft config to illustrate your idea?
As to deleting conntrack states that fall in blacklist, can you explain what's the goal?

  • in absence of nftables or selectable way to change overall engine to ip route blackhole

If I understand you correctly, you are proposing an alternative way to do geoip blocking for systems which don't have nftables? By redirecting matching packets to the blackhole route? This is probably doable but I think loading 60k-100k ip's 1 by 1 into the routing table will take a long while. Is there a way to load them in bulk? I've seen the 'Bird internet routing daemon' project which seems to allow this but I don't know much about it otherwise. Anyway, the first question I'm asking myself is how many users would actually want this? In this case, I'm not sure that would be many.

  1. whichever, just make sure it is not :00/5
    You can add snips from under table inet fw4 { !!!here!!!! } in /etc/nftables.d
chain mssfix {
    type filter hook postrouting priority mangle; policy accept;
    oif $wan_devices tcp option maxseg size set rt mtu

}

At same priority order is nondeterministic so suggestion to go less than 50 in ordering direction

Routes take subnets just like sets.

I'm trying really hard to decipher what it is you are suggesting exactly but so far I fail miserably :slight_smile:

If your point is to process geoip rules before built-in fw4 rules then this makes sense, and is already implemented. geoip-shell uses the netfilter hook prerouting with priority mangle. Generally all built-in fw4 rules use the netfilter hook 'input' or later, except these 2:

chain raw_prerouting {
		type filter hook prerouting priority raw; policy accept;
}

chain mangle_prerouting {
		type filter hook prerouting priority mangle; policy accept;
}

These are empty by default, so effectively geoip rules are processed prior to any built-in fw4 rules.

Also note that geoip-shell currently only filters ingress traffic, so I don't see how the postrouting hook and oif (output interfaces) in your example are relevant to the subject.

I may be misunderstanding you, and if that's the case then you'll need to help me by explaining your idea using longer/complete sentences.

Example was meant to be useless, you can define full hook before or after fw4 hooks since default ones may fill from user ruleset.

From my perspective, if an expert user explicitly wants to create rules which are processed very early in the chain then more power to them, I don't see why I would want to override their decision. To the contrary, I think it's a good thing that an expert user is able to add firewall rules preceding geoip rules if they choose to.

I can only hope that was lost in translation.

@brada as to your request

add random sleep in crontab script for like half of period

This is my proposed implementation in the -run script (which is called from the cron job). Does this satisfy your request?

# wait $reboot_sleep seconds after boot, or 0-300 seconds before updating
[ "$daemon_mode" ] && {
	if [ "$action_run" = restore ]; then
		IFS='. ' read -r uptime _ < /proc/uptime
		: "${uptime:=0}"
		: "${reboot_sleep:=30}"
		sl_time=$((reboot_sleep-uptime))
	elif [ "$action_run" = update ]; then
		rand_int="$(tr -cd 0-9 < /dev/urandom | dd bs=1 count=3 2>/dev/null)"
		: "${rand_int:=0}"
		sl_time=$((rand_int*300/999))
	fi
	[ $sl_time -gt 0 ] && {
		echolog "Sleeping for ${sl_time}s..."
		sleep $sl_time
	}
}

Whatever you chose will be good.

Since you requested a change in code (I'm speaking about random sleep time before update), I would be grateful if you confirmed that my implementation satisfies your request. If it doesn't then please let me know as well :slight_smile: