Code review comment for ~sergiodj/ubuntu/+source/clamav:ifupdown-replacement

Revision history for this message
Bryce Harrington (bryce) wrote :

I set up a vm and did the suggested test of adding debugging to clamav-freshclam:

set -e

echo "Running clamav-freshclam-ifupdown ${*}" >&2

[ -f /var/lib/clamav/interface ] || exit 0

if [ -d /run/systemd/system ]; then
    ...

I guess the test for the interface file causes the script to exit early so the debug statement seems to need to precede it, however it does appear to get called:

May 26 05:42:27 clamav-test-mantic systemd-timesyncd[11333]: No network connectivity, watching for changes.
May 26 05:42:33 clamav-test-mantic systemd-networkd[11337]: enp5s0: Link UP
May 26 05:42:33 clamav-test-mantic systemd-networkd[11337]: enp5s0: Gained carrier
May 26 05:42:33 clamav-test-mantic systemd-networkd[11337]: enp5s0: DHCPv4 address 10.69.244.74/24, gateway 10.69.244.1 acquired from 10.69.244.1
May 26 05:42:33 clamav-test-mantic systemd-timesyncd[11333]: Network configuration changed, trying to establish connection.
May 26 05:42:33 clamav-test-mantic networkd-dispatcher[15544]: Running clamav-freshclam-ifupdown
May 26 05:42:33 clamav-test-mantic systemd-timesyncd[11333]: Contacted time server 185.125.190.56:123 (ntp.ubuntu.com).
May 26 05:42:34 clamav-test-mantic systemd-networkd[11337]: enp5s0: Gained IPv6LL

I verified also the autopkgtests run fine, and did a cursory skim through the script's code. It all looks pretty straightforward and standardish, so +1 LGTM.

Regarding Steve's feedback to just make the script be networkd-only, honestly it may not be a terrible idea. Certainly a single codepath is going to be more testable and less code to maintain. If the goal is to avoid delta with debian, then an alternative might be to carry both versions of the script and then have the packaging system install the proper one based on the system configuration, rather than having the script heuristically guess which codepath to use. Anyway, I don't have a firm opinion on what would be better, either approach should work out ok.

review: Approve

« Back to merge proposal