core, udev: remove old device on move event

[yuwata]

Fixes #16967.

[keszybz]

Looks OK, but the udev rules files somehow doesn't feel right. @poettering do you have a better approach?

[yuwata]

CentOS CIs are failed. @mrc0mmand Is it related to that the CI uses an old image for the test?

[TASK START] Mon 07 Sep 2020 08:18:29 AM UTC
make: Entering directory '/build/test/TEST-29-UDEV-ID_RENAMING'
TEST-29-UDEV-ID_RENAMING SETUP: UDEV ID_RENAMING property
Reusing existing cached image /systemd-meson-build/test/TEST-29-UDEV-ID_RENAMING/../default.img → /systemd-meson-build/test/default.img
'/systemd-meson-build/test/default.img' -> '/var/tmp/systemd-test-TEST-29-UDEV-ID_RENAMING/default.img'

[yuwata]

Ah, IMAGE_NAME= needs to be set when non-basic image is used.

[poettering]

So, yea, I think we should change "move" actions to not copy the db, it sounds wrong to me if we#d copy it then.

[keszybz]

So, yea, I think we should change "move" actions to not copy the db, it sounds wrong to me if we#d copy it then.

Agreed.

[yuwata]

@poettering and @keszybz Thank you for the comments. Updated. Now udev properties are also flushed on 'move' uevent. However, it makes 'ID_NET_DRIVER=', 'ID_NET_LINK_FILE=' and 'ID_NET_NAME=' dropped after 'move' event. So, I changed 80-net-setup-link.rules to apply .link file also on 'move' event (strictly speaking, all event but except for 'remove' event, I am not sure what we should do e.g., 'change' or 'bind' event). PTAL.

[yuwata]

semaphoreci failed with internal compiler error. Seems not related.

FAILED: systemd-networkd 
cc  -o systemd-networkd systemd-networkd.p/src_network_networkd.c.o -flto -Wl,--as-needed -Wl,--no-undefined -pie -Wl,-z,relro -Wl,-z,now -fstack-protector -Wl,--gc-sections -g -O2 -fdebug-prefix-map=/tmp/autopkgtest-lxc.fpuyd49n/downtmp/build.naw/src=. -fstack-protector-strong -Wformat -Werror=format-security -Wl,-z,relro -Wl,--start-group src/network/libnetworkd-core.a src/shared/libsystemd-shared-246.so src/libsystemd-network/libsystemd-network.a src/udev/libudev_static.a src/shared/libsystemd-shared-246.a src/libsystemd/libsystemd_static.a src/basic/libbasic.a src/udev/libudev-basic.a -pthread -lacl /usr/lib/x86_64-linux-gnu/libblkid.so /usr/lib/x86_64-linux-gnu/libcap.so -lcrypt -lgcrypt /usr/lib/x86_64-linux-gnu/libidn2.so /usr/lib/x86_64-linux-gnu/libip4tc.so /usr/lib/x86_64-linux-gnu/libip6tc.so /usr/lib/x86_64-linux-gnu/libkmod.so /usr/lib/gcc/x86_64-linux-gnu/10/../../../x86_64-linux-gnu/liblz4.so /usr/lib/x86_64-linux-gnu/libmount.so /usr/lib/x86_64-linux-gnu/libssl.so /usr/lib/x86_64-linux-gnu/libcrypto.so -lpam -lrt /usr/lib/x86_64-linux-gnu/libseccomp.so /usr/lib/gcc/x86_64-linux-gnu/10/../../../x86_64-linux-gnu/libselinux.so /usr/lib/gcc/x86_64-linux-gnu/10/../../../x86_64-linux-gnu/libzstd.so /usr/lib/x86_64-linux-gnu/liblzma.so -lm -ldl -Wl,--end-group '-Wl,-rpath,$ORIGIN/src/shared' -Wl,-rpath-link,/tmp/autopkgtest-lxc.fpuyd49n/downtmp/build.naw/src/build-deb/src/shared
lto1: internal compiler error: output_operand: invalid expression as operand
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-10/README.Bugs> for instructions.
lto-wrapper: fatal error: cc returned 1 exit status
compilation terminated.
/usr/bin/ld: error: lto-wrapper failed
collect2: error: ld returned 1 exit status

[keszybz]

Hmm, so with your patch, we don't set ID_NET_NAME. But we don't know for sure if end-user rules or other consumers of the udev db don't make some other use of ID_NET_NAME. So... shouldn't we instead set ID_NET_NAME unconditionally, and tweak rules.d/80-net-setup-link.rules so simply skip the rename when ACTION!="add" ? I don't know what better.

[yuwata]

Updated. Now ID_NET_NAME= is also re-assigned. Note that interface renaming is done only on 'add' uevent:

systemd/src/udev/udev-event.c

Lines 834 to 835 in 12ce0f4

	if (!device_for_action(dev, DEVICE_ACTION_ADD))
	return 0; /* Rename the interface only when it is added. */

So, it is safe to set NAME= on all uevent in 80-net-setup-link.rules.

[poettering]

@poettering and @keszybz Thank you for the comments. Updated. Now udev properties are also flushed on 'move' uevent. However, it makes 'ID_NET_DRIVER=', 'ID_NET_LINK_FILE=' and 'ID_NET_NAME=' dropped after 'move' event. So, I changed 80-net-setup-link.rules to apply .link file also on 'move' event (strictly speaking, all event but except for 'remove' event, I am not sure what we should do e.g., 'change' or 'bind' event). PTAL.

So in the in NEWS file we actually suggest people should always use !="remove" rather than == add|change or so, hence this change is very much in line with what we suggest there, so that sounds good at first sight.

Hmm, but then again, the net setup stuff might readjust device parameters, and we probably shouldn't do that on "change" or so, but only on "add" (and maybe "bind"?) So maybe, the net setup code itself should check the event action, and if it's "add" or "bind" make device changes, but if it's anything else it should just generate props...

Hmm, that said: we probably should manually import specific props that we cannot regenerate on each event, like before b081b27. i.e. once a device is renamed and otherwise changed we might not be able to determine anymore whether it was us that renamed the thing and based on which .link file, hence we should probably import the props that is stored in over manually by focussed udev rules.

[yuwata]

@poettering Thank you for the comments. Updated.

Hmm, but then again, the net setup stuff might readjust device parameters, and we probably shouldn't do that on "change" or so, but only on "add" (and maybe "bind"?) So maybe, the net setup code itself should check the event action, and if it's "add" or "bind" make device changes, but if it's anything else it should just generate props...

Now, .link settings are applied on 'add', 'bind', and also 'move' events. On 'move' event, Name= and NamePolicy= settings are ignored, as it may re-generate 'move' event. But, I have no reason to not apply other settings on 'move' event.

Hmm, that said: we probably should manually import specific props that we cannot regenerate on each event, like before b081b27. i.e. once a device is renamed and otherwise changed we might not be able to determine anymore whether it was us that renamed the thing and based on which .link file, hence we should probably import the props that is stored in over manually by focussed udev rules.

Not sure. what is the difference between IMPORT{builtin}="net_setup_link" and just importing the three properties?

[yuwata]

@poettering and @keszybz Could you review this again?

[poettering]

Not sure. what is the difference between IMPORT{builtin}="net_setup_link" and just importing the three properties?

Hmm, the ID_NET_LINK_FILE gets dropped that caused the interface to be renamed. But maybe that's actually OK, since it probably should be set to whatever counts now...