On Wed, Nov 14, 2018 at 04:55:03PM -0500, Daniel Kahn Gillmor wrote: > a) is there a risk that we could somehow have the wireguard kernel > module loaded, but *not* have wireguard.ko available for finding via > modinfo -F ? If that's the case, then it looks like the postinst > script will exit with an error, rather than exiting cleanly.
indeed, after removing any and all wireguard.ko from the usual search paths: # new_version=$(modinfo -F version wireguard 2>/dev/null || echo 'invalid') # echo $new_version invalid # cat /sys/module/wireguard/version 0.0.20181018 this is likely to be triggered if wireguard-dkms fails to build the (new) module for the currently running kernel, since the old one was removed already. with the above and an additional conditional early exit, it should handle all those cases correctly. > b) is there a reason why we walk through $units in a for loop, when > systemctl is supposed to be able to take an arbitrary number of > units as arguments? > > that is, why not just do: > > systemctl stop $units || true > > instead of this whole rigamarole: > > for unit in $units; do > echo "$unit" >&2 > systemctl stop "$unit" >/dev/null || true > done needs an additional |tr '\n' ' ' when retrieveing the unit list, but should work. it puts trust into the systemctl output though, unless we want to sprinkle in some quoting magic (we cannot quote the whole $units string, because then systemctl thinks the whole thing is a wrongly escaped single unit name). > c) where does "sleep 3" come from? why 3 seconds instead of 1 or 5? > i'm always a bit leery of magic numbers like this. from the Ubuntu PPA version ;) we could also poll for some random interval (e.g., once per second for up to 5 or 10 seconds?) instead. I don't think there is a way to tell systemctl to return only after all the requested units have fully stopped, otherwise that would be the way to go. > d) is it possible to avoid stopping and restarting wg-quick@ units by > comparing it to "wg show" *before* stopping and starting wg-quick@ > units? If it's not too clever to do, it might at least avoid some > unnecessary network disruption on some systems. sure - if you mean ensuring that there are no non-wg-quick wireguard interfaces configured in the current netns, i.e. the interfaces returned by 'wg' must all have corresponding 'wg-quick@' instances, but not vice versa. wg might show less interfaces than there are running wg-quick@ instances, since the interfaces might get moved to their own netns after creation (wireguard continues to use the netns in which it was created for sending the encrypted traffic, making this an easy way to enforce tunneling for individual processes/cgroups/containers). > let me know what you think! thanks for being so gracious about the > back-and-forth here, and for your work on getting this into shape! sounds good. there's one extra thing that I noticed while looking at the postinst some more. the version check is just for inequality - it might make sense to compare it properly to ensure the 'new' module is actually newer than the 'old' one? I probably won't have much more time until Sunday for this, so feel free to take over (or not - I am happy to see this through till the end :-P). and no worries about the back-and-forth - it's a lot better than getting ignored or code being merged without discussion and review ;)
