On Thu 2018-11-15 14:45:10 +0100, Fabian Grünbichler wrote: > 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.
sounds like a reasonable approach. my main concern was that the
original new_version assignment (which didn't have the "|| echo
'invalid'" pipeline) would just fail, triggering the set -e.
>> 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' ' '
does it really? i think \n is typically part of $IFS, so i don't see
why this would be necessary.
> 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).
we're already cutting the line at the beginning of the first whitespace,
and then passing each element to "systemctl stop" individually, i'm not
sure what you think could go wrong here. This is using systemctl output
to talk to systemctl, right? are you concerned about shell
metacharacters? other whitespace? something else?
> 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.
By default, systemctl will verify, enqueue, and wait until the shtudown
is completed, as long as --no-block is not applied, if i'm reading
systemctl(1) correctly. isn't that sufficient? I'm not sure what the
--wait argument does for "systemctl stop", but maybe that could be
explored as well.
> 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).
i see, thanks for thinking about the namespaces. So yeah, the
preliminary check would avoid going through this whole process if it
detects something in the current namespaces, but it might go ahead and
try to run the process anyway if it missed something -- that seems fine
to me.
But your point about the namespaces makes me wonder whether it makes
sense to just "rmmod wireguard" in the middle of the "set -e" postinst
script -- won't that fail if there are active interfaces in some other
netns? if so, shouldn't the postinst handle ihis case gracefully?
>> 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'd say we should do that -- limit the teardown/reload to times when the
"new" module is actually newer than the "old" one. maybe use dpkg
--compare-versions to do that comparison?
All the best,
--dkg
signature.asc
Description: PGP signature
